✨ feat: implement fiber architecture with complete reconciliation eng… - #31
✨ feat: implement fiber architecture with complete reconciliation eng…#31MarcelOlsen wants to merge 12 commits into
Conversation
…ine and comprehensive documentation Replaces the old recursive rendering system with React-like fiber architecture for better performance and future concurrent mode support, includes full test suite, modernized ElysiaJS example app, and detailed design documentation for all core systems.
WalkthroughMajor refactor replacing the legacy VDOM reconciler with a complete Fiber-based renderer: new fiber core (types, flags, creation, root, work loop, begin/complete/commit phases), fiber-backed hooks/context/event wiring, many tests/docs, and an overhauled example to Bun/Elysia; legacy reconciler/dom-renderer removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Dev
participant API as MiniReact.render
participant WL as WorkLoop (scheduleUpdateOnFiber)
participant BW as beginWork
participant CW as completeWork
participant Commit as commitWork
participant DOM as Browser DOM
Dev->>API: render(element, container)
API->>WL: create/get FiberRoot & scheduleUpdateOnFiber
WL->>WL: renderRootSync (build WIP)
WL->>BW: beginWork(current, wip)
BW->>BW: reconcileChildren -> produce child fibers
BW-->>WL: next child or null
WL->>CW: completeWork(current, wip)
CW->>CW: create/update stateNode, set effect tags
WL->>WL: finish loop -> finishedWork
WL->>Commit: commitRoot(finishedWork)
Commit->>DOM: insert/remove/update nodes, run layout effects/refs
Commit-->>Dev: render complete
sequenceDiagram
autonumber
participant Component as Component
participant Hook as dispatchSetState
participant Queue as UpdateQueue
participant WL as WorkLoop
Component->>Hook: dispatchSetState(action)
Hook->>Queue: enqueue update (circular list)
Hook->>WL: scheduleUpdateOnFiber(ownerFiber)
WL->>WL: renderRootSync -> processUpdateQueue during render
WL->>Component: component receives updated state
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas I will be nitpicky about — review these closely:
Possibly related PRs
Poem
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (6)
example/src/types.d.ts (1)
1-1: Yo, chat, are we really doing triple-slash directives in 2025?Listen, I know this works and all, but we're out here creating an entire file just to triple-slash reference some types? That's like... that's like creating a whole new component just to wrap a div, you know what I'm saying?
Here's the thing - triple-slash directives are the TypeScript equivalent of using
varin JavaScript. They work, but they're old-school. Modern TypeScript has better ways to handle this:Option 1 (The Based Approach): Just add this to your
example/tsconfig.json:{ "include": [ "src/**/*", "../src/jsx.d.ts" ] }Option 2 (Also Valid): If you need the types globally available in your example app, you could add it to the
typesarray in tsconfig.This whole file could just... not exist. And that's beautiful, chat. Less code, more type safety, same result. Plus you're not maintaining a separate file that's literally one line of path magic.
But hey, it works, so if you wanna ship this and refactor later when you're not knee-deep in fiber architecture, I get it. Just putting it out there that we can do better.
example/src/server.ts (1)
213-213: Hardcoded port 3000 - make it configurable.Look, I get it, it's an example app. But hardcoding ports is how you end up with "port already in use" errors and confusion in different environments.
Do this:
+const PORT = Number(process.env.PORT) || 3000; + -.listen(3000); +.listen(PORT);Two lines. That's all it takes to make your life easier when you inevitably need to run this on a different port.
example/README.md (1)
112-121: Give the file tree fence a language.This naked triple backtick is pure chaos. markdownlint is already screaming (MD040), and adding a language keeps the docs tooling happy. Just slap a neutral label like
texton that block:-``` +```text example/ ├── src/ │ ├── server.ts # ElysiaJS server @@ └── README.md -``` +```Let’s keep the README as tidy as the code.
docs/03-reconciliation.md (1)
338-345: Slap a language on these fencesCome on, we’re better than raw triple-backticks floating in space. Markdown lint is already yelling because the example list of keyed vs unkeyed children lacks a language tag, and readers lose syntax highlighting. Tag it as
text(or whatever fits) so the docs stay as polished as the fiber architecture you just shipped.docs/05-hooks-system.md (1)
461-466: Give the update-queue diagram a language tagSame story here: the circular queue snippet is naked markdown. Toss a
text(or similar) language hint on that block so the lint bots chill and humans get consistent formatting. Easy win, no excuses.src/hooks/types.ts (1)
12-36: Stop nuking our generics onUpdateQueueDude, we just torched all the compile-time signal by slapping
UpdateQueue<unknown>onto state/reducer hooks. The whole point of threading these queues through the fiber types is to keep the actual state payload wired up. Keep it generic—UpdateQueue<T>for state hooks andUpdateQueue<State>for reducers—so downstream code doesn’t fall back toany-land. Patch it up like this:- queue?: UpdateQueue<unknown>; // Update queue for Fiber integration + queue?: UpdateQueue<T>; // Preserve the state type in the queue ... - queue?: UpdateQueue<unknown>; // Update queue for Fiber integration + queue?: UpdateQueue<State>; // Preserve reducer state typingThat way the scheduler keeps its TypeScript edge instead of devolving into shrug emoji types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
example/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (56)
.gitignore(1 hunks).trae/rules/project_rules.md(0 hunks)biome.json(2 hunks)docs/01-fiber-architecture.md(1 hunks)docs/02-work-loop.md(1 hunks)docs/03-reconciliation.md(1 hunks)docs/04-event-system.md(1 hunks)docs/05-hooks-system.md(1 hunks)docs/README.md(1 hunks)example/.gitignore(1 hunks)example/README.md(1 hunks)example/babel.config.js(0 hunks)example/biome.json(1 hunks)example/package.json(1 hunks)example/public/index.html(0 hunks)example/src/App.jsx(0 hunks)example/src/Counter.jsx(0 hunks)example/src/Modal.jsx(0 hunks)example/src/ReducerDemo.jsx(0 hunks)example/src/RefDemo.jsx(0 hunks)example/src/TodoList.jsx(0 hunks)example/src/app.tsx(1 hunks)example/src/index.js(0 hunks)example/src/server.ts(1 hunks)example/src/types.d.ts(1 hunks)example/tsconfig.json(1 hunks)example/webpack.config.js(0 hunks)package.json(1 hunks)src/MiniReact.ts(2 hunks)src/context/index.ts(3 hunks)src/core/index.ts(3 hunks)src/fiber/beginWork.ts(1 hunks)src/fiber/commitWork.ts(1 hunks)src/fiber/completeWork.ts(1 hunks)src/fiber/domOperations.ts(1 hunks)src/fiber/fiberCreation.ts(1 hunks)src/fiber/fiberFlags.ts(1 hunks)src/fiber/fiberHooks.ts(1 hunks)src/fiber/fiberRoot.ts(1 hunks)src/fiber/index.ts(1 hunks)src/fiber/reconcileChildren.ts(1 hunks)src/fiber/types.ts(1 hunks)src/fiber/workLoop.ts(1 hunks)src/hooks/fiberHooksImpl.ts(1 hunks)src/hooks/index.ts(1 hunks)src/hooks/index.ts.backup(1 hunks)src/hooks/types.ts(3 hunks)src/jsx-dev-runtime.ts(1 hunks)tests/MiniReact.jsx.test.ts(2 hunks)tests/fiber/beginWork.test.ts(1 hunks)tests/fiber/commitWork.test.ts(1 hunks)tests/fiber/completeWork.test.ts(1 hunks)tests/fiber/fiberCreation.test.ts(1 hunks)tests/fiber/integration.test.ts(1 hunks)tests/fiber/reconciliation.test.ts(1 hunks)tests/fiber/workLoop.test.ts(1 hunks)
💤 Files with no reviewable changes (11)
- .trae/rules/project_rules.md
- example/src/App.jsx
- example/src/index.js
- example/src/Counter.jsx
- example/src/Modal.jsx
- example/babel.config.js
- example/webpack.config.js
- example/src/RefDemo.jsx
- example/src/TodoList.jsx
- example/src/ReducerDemo.jsx
- example/public/index.html
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MarcelOlsen
PR: MarcelOlsen/mini-react#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 (23)
src/context/index.ts (3)
src/fiber/fiberHooks.ts (1)
getCurrentRenderingFiber(49-51)src/fiber/index.ts (1)
getCurrentRenderingFiber(103-103)src/hooks/index.ts (1)
getCurrentRenderingFiber(56-56)
src/fiber/fiberCreation.ts (3)
src/MiniReact.ts (5)
ElementType(11-11)AnyMiniReactElement(8-8)TEXT_ELEMENT(133-133)FRAGMENT(133-133)PORTAL(133-133)src/fiber/types.ts (2)
Props(23-28)Fiber(141-335)src/fiber/fiberFlags.ts (2)
NoEffect(15-15)NoLanes(42-42)
src/fiber/fiberRoot.ts (2)
src/fiber/types.ts (2)
FiberRoot(349-400)Fiber(141-335)src/fiber/fiberFlags.ts (1)
NoLanes(42-42)
tests/fiber/integration.test.ts (4)
src/fiber/fiberRoot.ts (1)
createFiberRoot(23-59)src/fiber/workLoop.ts (1)
scheduleUpdateOnFiber(39-46)src/MiniReact.ts (3)
TEXT_ELEMENT(133-133)FRAGMENT(133-133)AnyMiniReactElement(8-8)src/fiber/types.ts (1)
Fiber(141-335)
tests/fiber/beginWork.test.ts (3)
src/fiber/fiberCreation.ts (2)
createFiber(25-85)createWorkInProgress(102-161)src/fiber/beginWork.ts (1)
beginWork(44-81)src/fiber/fiberRoot.ts (1)
createFiberRoot(23-59)
example/src/app.tsx (6)
src/MiniReact.ts (11)
MiniReactContext(37-37)createContext(36-36)useState(17-17)useRef(20-20)MutableRefObject(32-32)useEffect(18-18)memo(84-99)useCallback(22-22)useMemo(21-21)useContext(36-36)render(6-6)src/context/index.ts (2)
createContext(33-80)useContext(87-114)src/hooks/fiberHooksImpl.ts (5)
useState(92-148)useRef(293-323)useEffect(241-285)useCallback(384-428)useMemo(332-375)src/hooks/types.ts (1)
MutableRefObject(113-115)src/portals/types.ts (1)
PortalElement(8-14)src/core/index.ts (1)
render(64-100)
src/fiber/workLoop.ts (6)
src/fiber/types.ts (2)
Fiber(141-335)FiberRoot(349-400)src/performance/index.ts (2)
trackRenderStart(44-48)trackRenderEnd(53-60)src/fiber/commitWork.ts (1)
commitRoot(36-59)src/fiber/fiberCreation.ts (1)
createWorkInProgress(102-161)src/fiber/beginWork.ts (1)
beginWork(44-81)src/fiber/completeWork.ts (1)
completeWork(39-67)
tests/fiber/reconciliation.test.ts (4)
src/fiber/fiberCreation.ts (1)
createFiber(25-85)src/fiber/reconcileChildren.ts (1)
reconcileChildren(83-102)src/fiber/fiberFlags.ts (2)
Placement(21-21)UpdateEffect(27-27)src/fiber/types.ts (1)
Fiber(141-335)
src/fiber/beginWork.ts (4)
src/fiber/types.ts (1)
Fiber(141-335)src/fiber/reconcileChildren.ts (1)
reconcileChildren(83-102)src/fiber/fiberCreation.ts (1)
createWorkInProgress(102-161)src/fiber/fiberHooks.ts (1)
setCurrentRenderingFiber(34-40)
tests/fiber/completeWork.test.ts (6)
src/fiber/fiberCreation.ts (2)
createFiber(25-85)createWorkInProgress(102-161)src/fiber/completeWork.ts (1)
completeWork(39-67)src/fiber/fiberFlags.ts (2)
Placement(21-21)UpdateEffect(27-27)src/fiber/fiberRoot.ts (1)
createFiberRoot(23-59)src/fiber/beginWork.ts (1)
beginWork(44-81)src/fiber/types.ts (1)
Fiber(141-335)
src/fiber/reconcileChildren.ts (4)
src/MiniReact.ts (2)
AnyMiniReactElement(8-8)TEXT_ELEMENT(133-133)src/fiber/types.ts (2)
Fiber(141-335)isSameElementType(511-516)src/fiber/fiberCreation.ts (4)
createFiberFromElement(171-219)getElementKey(335-353)getElementType(389-411)createWorkInProgress(102-161)src/fiber/fiberFlags.ts (3)
Placement(21-21)Deletion(35-35)UpdateEffect(27-27)
src/fiber/commitWork.ts (3)
src/fiber/types.ts (4)
FiberRoot(349-400)Fiber(141-335)PortalContainer(341-343)RefObject(15-15)src/fiber/fiberFlags.ts (2)
Placement(21-21)UpdateEffect(27-27)src/fiber/domOperations.ts (4)
insertBefore(23-33)updateTextContent(147-149)updateProperties(83-139)removeChild(41-43)
src/hooks/fiberHooksImpl.ts (3)
src/hooks/types.ts (12)
UseStateHook(71-74)StateHook(8-13)StateOrEffectHook(62-69)Reducer(86-86)ReducerHook(30-36)EffectCallback(77-77)DependencyList(78-78)EffectHook(15-22)MutableRefObject(113-115)RefHook(38-41)MemoHook(43-48)CallbackHook(50-56)src/fiber/fiberHooks.ts (7)
getCurrentRenderingFiber(49-51)createUpdateQueue(62-72)processUpdateQueue(151-182)dispatchSetState(110-140)processReducerQueue(236-270)dispatchReducerAction(195-225)areDepsEqual(282-304)src/fiber/types.ts (1)
UpdateQueue(79-116)
tests/fiber/fiberCreation.test.ts (4)
src/fiber/fiberCreation.ts (9)
createFiber(25-85)createWorkInProgress(102-161)createFiberFromElement(171-219)createFiberFromText(227-240)createFiberFromFragment(252-265)cloneFiber(305-313)getElementKey(335-353)getElementProps(361-381)getElementType(389-411)src/fiber/fiberFlags.ts (2)
NoEffect(15-15)NoLanes(42-42)src/MiniReact.ts (2)
TEXT_ELEMENT(133-133)FRAGMENT(133-133)src/hooks/types.ts (1)
StateOrEffectHook(62-69)
tests/fiber/workLoop.test.ts (4)
src/fiber/fiberRoot.ts (1)
createFiberRoot(23-59)src/fiber/fiberCreation.ts (1)
createFiber(25-85)src/fiber/workLoop.ts (2)
scheduleUpdateOnFiber(39-46)getCurrentFiber(282-284)src/MiniReact.ts (3)
AnyMiniReactElement(8-8)FRAGMENT(133-133)PORTAL(133-133)
src/hooks/types.ts (1)
src/fiber/types.ts (1)
UpdateQueue(79-116)
tests/fiber/commitWork.test.ts (6)
src/fiber/types.ts (1)
FiberRoot(349-400)src/fiber/fiberRoot.ts (1)
createFiberRoot(23-59)src/fiber/fiberCreation.ts (2)
createFiber(25-85)createWorkInProgress(102-161)src/MiniReact.ts (1)
TEXT_ELEMENT(133-133)src/fiber/fiberFlags.ts (3)
Placement(21-21)UpdateEffect(27-27)Deletion(35-35)src/fiber/commitWork.ts (1)
commitRoot(36-59)
src/fiber/fiberHooks.ts (3)
src/fiber/types.ts (3)
Fiber(141-335)UpdateQueue(79-116)Update(54-69)src/hooks/index.ts (3)
setCurrentRenderingFiber(55-55)setCurrentRenderingFiber(57-57)getCurrentRenderingFiber(56-56)src/fiber/workLoop.ts (1)
scheduleUpdateOnFiber(39-46)
src/fiber/completeWork.ts (4)
src/fiber/types.ts (1)
Fiber(141-335)src/fiber/fiberFlags.ts (2)
UpdateEffect(27-27)Placement(21-21)src/fiber/domOperations.ts (1)
setInitialProperties(51-72)src/core/types.ts (1)
VDOMInstance(98-107)
src/fiber/fiberFlags.ts (1)
src/fiber/types.ts (2)
EffectTag(34-34)Lanes(41-41)
src/fiber/domOperations.ts (1)
src/fiber/types.ts (1)
Props(23-28)
src/fiber/types.ts (3)
src/fiber/index.ts (23)
RefObject(17-17)RefCallback(18-18)Ref(16-16)Props(15-15)EffectTag(12-12)Lanes(13-13)SuspenseState(14-14)Update(19-19)UpdateQueue(20-20)Fiber(10-10)FiberRoot(11-11)isFiber(23-23)isFiberHostComponent(24-24)isFiberFunctionComponent(25-25)isFiberText(26-26)isFiberFragment(27-27)isFiberPortal(28-28)isFiberRoot(29-29)fiberHasEffect(30-30)fiberHasChildEffects(31-31)isSameType(32-32)isSameElementType(33-33)fiberNeedsRef(34-34)src/MiniReact.ts (2)
AnyMiniReactElement(8-8)ElementType(11-11)src/hooks/types.ts (1)
StateOrEffectHook(62-69)
src/core/index.ts (3)
src/fiber/types.ts (1)
FiberRoot(349-400)src/fiber/fiberRoot.ts (3)
hasFiberRoot(140-142)getFiberRoot(130-132)createFiberRoot(23-59)src/fiber/workLoop.ts (1)
scheduleUpdateOnFiber(39-46)
🪛 ast-grep (0.39.6)
src/fiber/fiberRoot.ts
[warning] 195-195: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: container.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
src/core/index.ts
[warning] 97-97: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: containerNode.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 LanguageTool
docs/02-work-loop.md
[style] ~355-~355: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...(does workInProgress become null)? 3. Is the effect list being built correctly? ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~356-~356: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e effect list being built correctly? 4. Is commit running after render completes? ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~377-~377: ‘overall structure’ might be wordy. Consider a shorter alternative.
Context: ...ures later without major changes to the overall structure.
(EN_WORDINESS_PREMIUM_OVERALL_STRUCTURE)
docs/04-event-system.md
[style] ~51-~51: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...ent` is cached so we can add/remove the exact same function reference. ## Initialization ...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
docs/03-reconciliation.md
[style] ~228-~228: Consider using “who” when you are referring to people instead of objects.
Context: ...as PLACEMENT) - Delete any old children that don't match ## Array Reconciliation T...
(THAT_WHO)
[style] ~445-~445: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ..., we can reuse the fiber. Otherwise, we need to create a new one. ## Deletion When a ...
(REP_NEED_TO_VB)
docs/05-hooks-system.md
[style] ~164-~164: Consider a more concise word here.
Context: ...ng = null } ``` We process all updates in order to compute the new state. ## useEffect S...
(IN_ORDER_TO_PREMIUM)
docs/README.md
[style] ~177-~177: This wording could be more concise.
Context: ... performance: O(n) Reconciliation: Linear in number of children Effect Lists: Only pro...
(ADJECTIVE_IN_ATTRIBUTE)
🪛 markdownlint-cli2 (0.18.1)
example/README.md
112-112: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/02-work-loop.md
245-245: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/03-reconciliation.md
338-338: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
407-407: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/05-hooks-system.md
461-461: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/01-fiber-architecture.md
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/README.md
110-110: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (10)
example/.gitignore (1)
1-4: LGTM! Clean gitignore setup.Yo, this is exactly what you want in a gitignore. Build artifacts (*.js, *.map), dependencies (node_modules), and that .DS_Store for the Mac devs. Chef's kiss. No weird patterns, no over-engineering. Just the essentials.
.gitignore (1)
178-178: LGTM!Just formatting, no functional changes. Moving on.
tests/MiniReact.jsx.test.ts (2)
1-1: YES! Adding afterEach for cleanup.Look at you importing afterEach! This is the setup for proper test cleanup. Let's see what you do with it...
19-24: THIS is proper test hygiene! LOVE IT!BRO! You're cleaning up the DOM after each test! This is exactly what you should be doing. No more test pollution, no more "why does this test pass individually but fail in the suite" nonsense. You're removing the container from the DOM, checking parentNode first - defensive coding at its finest.
This is the kind of test discipline that separates the pros from the "works on my machine" crowd. Respect.
example/src/server.ts (1)
1-12: Static file serving looks solid.The /app.js route is clean. Serving with Bun.file, proper content-type, cache headers. This is how you do it. No complaints here.
example/biome.json (1)
1-32: Clean biome config.Standard biome setup - nothing wild here. Formatter enabled, linter enabled with recommended rules, ignoring the right directories. Tab indentation with width 2, double quotes, trailing commas - all reasonable choices.
This is exactly what a biome.json should look like. No over-configuration, no weird rules. Just good defaults.
src/jsx-dev-runtime.ts (1)
6-6: Nice - exposing jsxDEV as a direct named export.So you were only exporting jsxDEV through the jsx and jsxs aliases, but now you're also exporting it directly. Smart move. This gives consumers the flexibility to import jsxDEV explicitly if they need it for debugging or tooling purposes, while maintaining backward compatibility with the aliases.
Clean API expansion. No breaking changes. This is how you do it.
biome.json (3)
16-17: Good call ignoring example/public.Yeah, you don't want built artifacts and generated files in your lint/format runs. Clean.
38-40: noNonNullAssertion as error - I LOVE this.YES! Making non-null assertions an error in production code. This is the energy we need. Non-null assertions (the
!operator) are basically you telling TypeScript "trust me bro" and we all know how that ends.If you think something can't be null, prove it with proper type guards or optional chaining. Don't just bang it away with
!and hope for the best.This is the kind of strict TypeScript configuration that keeps your codebase clean and prevents runtime nullability bugs.
43-54: Test override for noNonNullAssertion - pragmatic choice.So you're disabling the non-null assertion rule specifically for test files. I get it. In tests, you're often asserting on things you know exist because you just set them up. Using non-null assertions in test code to clean up noise is reasonable.
BUT - and I'm watching you here - don't let this become an excuse for sloppy test setup. If you find yourself banging away with
!all over your tests, that's a smell that your test fixtures might need better types or setup.This is fine for now, but keep it clean.
…test\n stability\n\n Address multiple issues identified in code review:\n\n **Build & CI**\n - Add typecheck to prepublishOnly pipeline to catch type errors before\n publishing\n\n **Core Fixes**\n - Fix ref lifecycle: detach old refs before attaching new ones when ref\n identity changes\n - Fix memo bailout sibling chain corruption by cloning entire child list\n instead of just first child\n - Convert EffectTag from string literals to bitwise flags to support combining\n multiple effects (Placement + Update)\n - Fix effect tag assignments to use |= instead of = to preserve existing flags\n (e.g., Placement)\n - Fix createFiberFromElement to return null for null/undefined elements (valid\n "render nothing" case)\n - Fix double-deletion bug by checking Deletion flag before calling deleteChild\n\n **Rendering Fixes**\n - Change truthy checks to nullish checks (?? and != null) to preserve valid\n falsy renderables (0, "", false)\n - Remove manual DOM clearing (innerHTML) to let fiber commit phase handle \n deletions properly via deletion list\n\n **Test Stability**\n - Remove all wall-clock timing assertions (performance.now() +\n expect(duration).toBeLessThan())\n - Replace with deterministic structural assertions (node counts, depth\n verification, effect chains)\n - Eliminated 11 flaky timing checks across beginWork, completeWork,\n integration, reconciliation, and workLoop tests
There was a problem hiding this comment.
Actionable comments posted: 14
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (30)
docs/01-fiber-architecture.md(1 hunks)docs/02-work-loop.md(1 hunks)docs/03-reconciliation.md(1 hunks)docs/05-hooks-system.md(1 hunks)docs/README.md(1 hunks)example/README.md(1 hunks)example/src/server.ts(1 hunks)example/src/templates/index.html(1 hunks)example/tsconfig.json(1 hunks)package.json(1 hunks)src/core/index.ts(3 hunks)src/events/eventSystem.ts(1 hunks)src/fiber/beginWork.ts(1 hunks)src/fiber/commitWork.ts(1 hunks)src/fiber/completeWork.ts(1 hunks)src/fiber/fiberCreation.ts(1 hunks)src/fiber/fiberFlags.ts(1 hunks)src/fiber/fiberRoot.ts(1 hunks)src/fiber/reconcileChildren.ts(1 hunks)src/fiber/types.ts(1 hunks)src/hooks/fiberHooksImpl.ts(1 hunks)src/hooks/types.ts(3 hunks)tests/MiniReact.falsyValues.test.ts(1 hunks)tests/fiber/beginWork.test.ts(1 hunks)tests/fiber/completeWork.test.ts(1 hunks)tests/fiber/effectTags.test.ts(1 hunks)tests/fiber/fiberCreation.test.ts(1 hunks)tests/fiber/integration.test.ts(1 hunks)tests/fiber/reconciliation.test.ts(1 hunks)tests/fiber/workLoop.test.ts(1 hunks)
🧰 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 (18)
tests/fiber/workLoop.test.ts (3)
src/fiber/fiberRoot.ts (1)
createFiberRoot(23-59)src/fiber/fiberCreation.ts (1)
createFiber(25-85)src/fiber/workLoop.ts (2)
scheduleUpdateOnFiber(39-46)getCurrentFiber(282-284)
src/fiber/commitWork.ts (4)
src/fiber/types.ts (4)
FiberRoot(350-401)Fiber(142-336)PortalContainer(342-344)RefObject(15-15)src/fiber/fiberFlags.ts (3)
hasEffectTag(75-77)Placement(22-22)UpdateEffect(28-28)src/MiniReact.ts (2)
PORTAL(133-133)TEXT_ELEMENT(133-133)src/fiber/domOperations.ts (4)
insertBefore(23-33)updateTextContent(147-149)updateProperties(83-139)removeChild(41-43)
tests/fiber/completeWork.test.ts (6)
src/fiber/fiberCreation.ts (2)
createFiber(25-85)createWorkInProgress(102-161)src/fiber/completeWork.ts (1)
completeWork(39-67)src/fiber/fiberFlags.ts (2)
Placement(22-22)UpdateEffect(28-28)src/fiber/fiberRoot.ts (1)
createFiberRoot(23-59)src/fiber/beginWork.ts (1)
beginWork(44-81)src/fiber/types.ts (1)
Fiber(142-336)
tests/fiber/beginWork.test.ts (3)
src/fiber/fiberCreation.ts (2)
createFiber(25-85)createWorkInProgress(102-161)src/fiber/beginWork.ts (1)
beginWork(44-81)src/fiber/fiberRoot.ts (1)
createFiberRoot(23-59)
tests/fiber/effectTags.test.ts (1)
src/fiber/fiberFlags.ts (1)
hasEffectTag(75-77)
src/fiber/reconcileChildren.ts (3)
src/fiber/types.ts (2)
Fiber(142-336)isSameElementType(512-517)src/fiber/fiberCreation.ts (4)
createFiberFromElement(220-270)getElementKey(386-404)getElementType(440-462)createWorkInProgress(102-161)src/fiber/fiberFlags.ts (4)
Placement(22-22)hasEffectTag(75-77)Deletion(36-36)UpdateEffect(28-28)
tests/fiber/fiberCreation.test.ts (3)
src/fiber/fiberCreation.ts (9)
createFiber(25-85)createWorkInProgress(102-161)createFiberFromElement(220-270)createFiberFromText(278-291)createFiberFromFragment(303-316)cloneFiber(356-364)getElementKey(386-404)getElementProps(412-432)getElementType(440-462)src/fiber/fiberFlags.ts (3)
NoEffect(16-16)NoLanes(43-43)UpdateEffect(28-28)src/hooks/types.ts (1)
StateOrEffectHook(62-69)
src/fiber/completeWork.ts (4)
src/fiber/types.ts (1)
Fiber(142-336)src/fiber/fiberFlags.ts (2)
UpdateEffect(28-28)Placement(22-22)src/fiber/domOperations.ts (1)
setInitialProperties(51-72)src/core/types.ts (1)
VDOMInstance(98-107)
tests/fiber/reconciliation.test.ts (4)
src/fiber/fiberCreation.ts (1)
createFiber(25-85)src/fiber/reconcileChildren.ts (1)
reconcileChildren(83-102)src/fiber/fiberFlags.ts (2)
Placement(22-22)UpdateEffect(28-28)src/fiber/types.ts (1)
Fiber(142-336)
src/fiber/types.ts (3)
src/MiniReact.ts (2)
AnyMiniReactElement(8-8)ElementType(11-11)src/fiber/fiberFlags.ts (1)
Update(30-30)src/hooks/types.ts (1)
StateOrEffectHook(62-69)
src/hooks/fiberHooksImpl.ts (3)
src/hooks/types.ts (12)
UseStateHook(71-74)StateHook(8-13)StateOrEffectHook(62-69)Reducer(86-86)ReducerHook(30-36)EffectCallback(77-77)DependencyList(78-78)EffectHook(15-22)MutableRefObject(113-115)RefHook(38-41)MemoHook(43-48)CallbackHook(50-56)src/fiber/fiberHooks.ts (7)
getCurrentRenderingFiber(49-51)createUpdateQueue(62-72)processUpdateQueue(151-182)dispatchSetState(110-140)processReducerQueue(236-270)dispatchReducerAction(195-225)areDepsEqual(282-304)src/fiber/types.ts (1)
UpdateQueue(80-117)
src/fiber/fiberRoot.ts (2)
src/fiber/types.ts (2)
FiberRoot(350-401)Fiber(142-336)src/fiber/fiberFlags.ts (1)
NoLanes(43-43)
src/fiber/beginWork.ts (4)
src/fiber/types.ts (1)
Fiber(142-336)src/fiber/reconcileChildren.ts (1)
reconcileChildren(83-102)src/fiber/fiberCreation.ts (1)
cloneChildFibers(174-210)src/fiber/fiberHooks.ts (1)
setCurrentRenderingFiber(34-40)
tests/fiber/integration.test.ts (4)
src/fiber/fiberRoot.ts (1)
createFiberRoot(23-59)src/fiber/workLoop.ts (1)
scheduleUpdateOnFiber(39-46)src/MiniReact.ts (3)
TEXT_ELEMENT(133-133)FRAGMENT(133-133)AnyMiniReactElement(8-8)src/fiber/types.ts (1)
Fiber(142-336)
src/fiber/fiberFlags.ts (2)
src/fiber/index.ts (18)
NoEffect(39-39)EffectTag(12-12)Placement(40-40)UpdateEffect(41-41)Update(19-19)Deletion(42-42)NoLanes(43-43)Lanes(13-13)SyncLane(44-44)DefaultLane(45-45)IdleLane(46-46)isEffectTagMutation(47-47)includesSomeLane(48-48)mergeLanes(49-49)removeLanes(50-50)isSubsetOfLanes(51-51)getHighestPriorityLane(52-52)includesSyncLane(53-53)src/fiber/types.ts (3)
EffectTag(35-35)Update(55-70)Lanes(42-42)
src/hooks/types.ts (1)
src/fiber/types.ts (1)
UpdateQueue(80-117)
src/core/index.ts (3)
src/fiber/types.ts (1)
FiberRoot(350-401)src/fiber/fiberRoot.ts (3)
hasFiberRoot(140-142)getFiberRoot(130-132)createFiberRoot(23-59)src/fiber/workLoop.ts (1)
scheduleUpdateOnFiber(39-46)
src/fiber/fiberCreation.ts (3)
src/MiniReact.ts (5)
ElementType(11-11)AnyMiniReactElement(8-8)TEXT_ELEMENT(133-133)FRAGMENT(133-133)PORTAL(133-133)src/fiber/types.ts (2)
Props(23-28)Fiber(142-336)src/fiber/fiberFlags.ts (2)
NoEffect(16-16)NoLanes(43-43)
🪛 ast-grep (0.39.7)
src/fiber/fiberRoot.ts
[warning] 195-195: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: container.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 LanguageTool
docs/02-work-loop.md
[style] ~355-~355: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...(does workInProgress become null)? 3. Is the effect list being built correctly? ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~356-~356: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e effect list being built correctly? 4. Is commit running after render completes? ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~377-~377: ‘overall structure’ might be wordy. Consider a shorter alternative.
Context: ...ures later without major changes to the overall structure.
(EN_WORDINESS_PREMIUM_OVERALL_STRUCTURE)
docs/05-hooks-system.md
[style] ~164-~164: Consider a more concise word here.
Context: ...ng = null } ``` We process all updates in order to compute the new state. ## useEffect S...
(IN_ORDER_TO_PREMIUM)
docs/03-reconciliation.md
[style] ~228-~228: Consider using “who” when you are referring to people instead of objects.
Context: ...as PLACEMENT) - Delete any old children that don't match ## Array Reconciliation T...
(THAT_WHO)
[style] ~445-~445: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ..., we can reuse the fiber. Otherwise, we need to create a new one. ## Deletion When a ...
(REP_NEED_TO_VB)
docs/README.md
[style] ~177-~177: This wording could be more concise.
Context: ... performance: O(n) Reconciliation: Linear in number of children Effect Lists: Only pro...
(ADJECTIVE_IN_ATTRIBUTE)
🪛 markdownlint-cli2 (0.18.1)
docs/README.md
7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
12-12: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
21-21: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
64-64: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
81-81: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
84-84: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
91-91: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
97-97: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
101-101: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
104-104: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
107-107: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (32)
package.json (1)
45-46: YOOO, this is CLEAN! Past review concern properly addressed.The typecheck script is now integrated into prepublishOnly exactly as requested. No more vibes-based type safety - this will catch type errors before publish. Beautiful follow-through on the feedback.
src/events/eventSystem.ts (1)
672-698: Portal cleanup implementation is PERFECT. This is how you prevent memory leaks.The symmetry here is beautiful - you add portal containers with
addEventDelegation, you clean them up incleanup. The loop at lines 684-690 removes all delegated listeners from each portal container, and line 694 clears the set.This is exactly what you want to see. No orphaned listeners, no memory leaks, proper resource management. Chef's kiss.
docs/05-hooks-system.md (1)
1-649: This hooks documentation is INSANE. Like, actually incredible.The depth here is perfect for an educational project. You're not just explaining WHAT hooks do, you're showing HOW they work internally - the hook array, the cursor, the update queues, the circular linked lists. This is the content that makes people understand React, not just use it.
The code examples are clean, the explanations are thorough, and you even covered the gotchas (stale closures, missing deps, wrong number of hooks). The Rules of Hooks section explains WHY the rules exist, not just WHAT they are.
Only thing I'd verify: make sure these code examples actually match your implementation. If someone's reading this to understand your codebase and the implementation differs, that's gonna be confusing.
example/README.md (2)
1-179: Example README is CLEAN. This is how you showcase a library.The transformation from basic example to comprehensive showcase is perfect. You're demonstrating:
- All the hooks
- Portals with proper event bubbling
- Context API
- Type safety throughout
The Bun + ElysiaJS stack is modern and fast. Good choice. The README explains what each demo does and why it matters. The Type Safety section showing how to properly type events and refs? Chef's kiss.
Only other nitpick: The Browser Support section (lines 169-176) listing specific version numbers might get stale. Maybe just say "Modern browsers with ES2020+ support" and link to caniuse or something.
70-77: Claim is verified as accurate - nice TypeScript discipline here.The verification confirms "No
anytypes used" is legitimately true across bothexample/src/app.tsxandexample/src/server.ts. Every hook has explicit generics (useState<number>,useRef<HTMLInputElement | null>), interfaces are properly defined (ThemeContextType,Todo,ExpensiveComponentProps), and contexts are typed at the point of creation.The
@ts-expect-errorcomments you've got are the right way to handle MiniReact/React type incompatibilities—they're notanyescape hatches, they're deliberate suppressions with context. That's the type-safe way to do it.No action needed. This README statement holds up.
docs/03-reconciliation.md (1)
1-550: This reconciliation doc is ABSOLUTELY CRACKED.The deep dive into the O(n) algorithm is exactly what people need to understand React's performance story. You explained:
- Why it's O(n) not O(n³)
- The lastPlacedIndex trick for movement detection (lines 382-418)
- The keyed vs unkeyed mixing problem (lines 332-351)
- Type matching logic
- The mount vs update split optimization
The code examples are detailed enough to be useful but not overwhelming. The diagrams at line 407-416 showing how lastPlacedIndex works? Perfect.
The "Debugging Tips" section at line 508-525 is clutch - this is what people actually need when reconciliation goes wrong.
This is the kind of documentation that makes a project legendary. It's not just "here's how to use it" - it's "here's how it ACTUALLY works."
example/tsconfig.json (1)
3-23: YOOO you turned on strict mode! This is the way.Line 12:
"strict": true- this is BASED. Too many projects skip this and then wonder why their types are lying to them. Strict mode is the only way to do TypeScript properly.The other changes are solid too:
- ESNext target/lib: You're using Bun, might as well use modern features
moduleResolution: "bundler": Perfect for Bun-based workflowsallowImportingTsExtensions: Necessary with bundler resolution- Path mappings (lines 19-23): Clean way to dev against local source
The jsxImportSource change from "mini-react" to "@marcelolsen/mini-react" aligns with the published package name.
This config is tight. No notes.
docs/01-fiber-architecture.md (1)
1-254: This fiber architecture doc is the PERFECT introduction. This is how you onboard people.You're starting with the fundamentals - what is a Fiber, why do we have two trees, what are the phases - and building up from there. The progression is logical and clear.
Highlights:
- The Fiber interface at lines 12-43 with inline comments is great reference material
- The ASCII diagram at lines 56-69 showing the double-buffer pattern? Clean
- The explanation of why we have two trees (lines 71-76): error recovery, concurrent mode ready, efficient updates - these are the RIGHT reasons
- The tree traversal code at lines 124-158 showing how to do depth-first without recursion is GOLD
The "Priority and Scheduling" section at lines 213-228 is good forward-looking content - shows the architecture supports features you haven't built yet.
This doc sets up the rest of the documentation perfectly. Someone reading this first will understand the "why" behind the implementation details in the other docs.
src/fiber/types.ts (6)
51-70: LGTM on the Update queue structure!Nice circular linked list setup here. The
action: State | ((prev: State) => State)is clean - you're handling both direct state updates and updater functions. The per-update lane priority is chef's kiss for concurrent mode later.One tiny thing though: there's no runtime validation that
nextactually forms a valid circular list. If you accidentally break the circle or create an infinite non-circular chain, you're gonna have a bad time debugging it. Might want to add a dev-mode validator.
72-117: UpdateQueue is CLEAN architecture!Okay so you've got the pending circular list for incoming updates, AND you've got the base queue for skipped low-priority updates. This is the real deal concurrent mode stuff. The separation between
lastRenderedState(optimization) andbaseState(correctness) shows you understand the priority system implications.The
lastRenderedReducersignature being(state: State, action: State | ((prev: State) => State)) => Stateis perfect - works for both useState (identity reducer) and useReducer (custom reducer).
142-336: This Fiber interface is THICC but well-organized.40+ fields in one interface? That's a lot, my guy. But I'll give you credit - the organization with comments (
// ===== IDENTITY =====, etc.) makes it navigable. The ASCII tree diagram at line 125-136 is chef's kiss - more code should have diagrams like this.Few observations:
memoizedState: unknownat line 238 - you've completely given up type safety here. I get it, different component types have different state shapes, butunknownmeans zero compile-time help. Consider a union type or generics if possible.
hooks: StateOrEffectHook<unknown>[] | nullat line 282 - same issue. That<unknown>is doing a lot of heavy lifting (or rather, NO lifting).The
stateNode: Node | FiberRoot | PortalContainer | nullunion at line 275 - this is actually good! Shows proper type discrimination.But here's the thing - this is a LOT of state in one place. Have you considered if any of these fields could be optional or moved to sub-structures? Like error boundary stuff, suspense stuff, concurrent mode stuff could be their own objects?
The size is manageable for now but keep an eye on it as you add more features. Also verify the
unknowntypes don't bite you later.
350-401: FiberRoot is solid but has some sus unknowns.Line 380:
callbackNode: unknown- okay so what IS this actually? A timeout ID? A promise? A scheduler task? The lack of type information here is rough. At least add a comment explaining what goes in here.Lines 390-395:
eventTimes: number[]andexpirationTimes: number[]- these arrays are gonna grow over time. Do you have a strategy for cleaning them up? Or do they just accumulate forever until the component unmounts? Might want to document the lifecycle of these arrays.Line 400:
pendingPassiveEffects?: Fiber[]- this is optional but the UpdateQueue stuff wasn't. Is there a reason for the inconsistency? Are some roots not gonna have passive effects?Otherwise the double-buffering with
currentandfinishedWorkis exactly right. That's the React pattern and you're following it well.
488-507: isSameType is critical for reconciliation and it's solid.This is the heart of whether you can reuse a fiber or need to create a new one. Type AND key must match (or both be null). The strict equality checks are correct here.
One edge case though: line 497
if (fiber1.type !== fiber2.type)works fine for strings and symbols, but for function components, you're comparing function references. If someone recreates the component function (like in hot reloading), this will fail to match even though it's "the same" component conceptually. Not much you can do about this without a more sophisticated system though.
522-524: fiberNeedsRef is clean.Only host components (DOM elements) get refs attached. Function components can't have refs unless you do forwardRef shenanigans. This is correct.
src/fiber/fiberFlags.ts (1)
10-36: Bitwise flags are CLEAN!Binary literals (
0b0001,0b0010, etc.) are chef's kiss for readability. Much better than hex or decimal magic numbers. Each flag is a power of 2 so you can OR them together.Line 30:
export const Update = UpdateEffect;with comment "Keep old name for backwards compatibility temporarily" - okay so TEMPORARILY for how long? Is there a TODO to remove this? Is there a migration plan? Or is "temporarily" just gonna become "forever" like it does in every codebase?Either commit to keeping both exports long-term, or create a migration plan to remove
Update. Don't let temporary aliases become permanent cruft.src/hooks/types.ts (2)
8-13: StateHook queue integration is solid!Adding
queue?: UpdateQueue<T>to StateHook is the right way to bridge hooks with the fiber update system. The optional nature means old code still works while fiber-based code can use the queue.The comment "Update queue for Fiber integration" is helpful. Same pattern repeated for ReducerHook at line 35.
One question though: what happens if a hook has BOTH a direct
setStatefunction AND a queue? Can they get out of sync? Or does one always delegate to the other?
15-22: needsRun flag makes sense for commit-phase effects.Adding
needsRun?: booleanto EffectHook is clean. During render you mark which effects need to run, then in commit phase you actually run them. This separation is exactly right for the fiber architecture.tests/fiber/workLoop.test.ts (5)
141-155: Deep nesting test is CRITICAL and you nailed it!Testing 100 levels of nesting (line 147-149) is exactly what separates iterative fiber from recursive rendering. With recursion, you'd blow the stack. With fibers, this should work fine.
This test proves your architecture works at scale. Love it.
284-312: Effect list order test is good defensive programming.Walking the nextEffect linked list to count effects (lines 303-308) validates that the effect list is built correctly. This is important because effect list bugs would cause commits to miss updates or crash.
The test expects
effectCount > 0which is a bit weak - you could assert the exact count is 3 (for the 3 divs at lines 291-293). But I get it, implementation details might vary.
360-381: WIP reuse test simulates the full cycle - nice!Lines 364-376 simulate a full render cycle:
- First render
- Commit (line 371:
root.current = firstWIP)- Second render
- Validate reuse
This tests the heart of fiber architecture - reusing fibers via the alternate pointer. The double-buffering is what makes fiber performant.
411-434: Error handling tests are ESSENTIAL!Line 416: orphaned fiber (no path to root) should throw - correct!
Line 433: component error propagates - also correct!These tests validate that broken states fail fast rather than silently corrupting the tree. That's defensive programming at its finest.
The comment at line 432 "error boundaries in Phase 7 will catch this" is a good roadmap hint.
513-569: Performance tests got FIXED - thank you!Previous review complained about wall-clock timing assertions (
< 100ms, etc) being flaky. You fixed it! Now you're validating structure:
- Line 532-538: count siblings, expect 100
- Line 560-567: count depth, expect 51
This is MUCH better. Structural validation is deterministic and doesn't flake on slow CI. The tests still exercise performance scenarios (large trees) but validate correctness instead of timing.
This is the right way to do it!
example/src/templates/index.html (1)
1-191: HTML template is clean for an example.The embedded CSS (lines 7-184) is fine for a showcase/example app. It's self-contained, which is good for demos. If this were production, I'd want external stylesheets, but for an example it's totally acceptable.
Nice touches:
- Gradient background (line 16)
- Clean card design (lines 26-32)
- Smooth button transitions (lines 55-68)
- Portal-specific positioning (lines 133-143)
The two mount points (
#rootand#portal-root) are clear and well-named.Only tiny nitpick: the viewport meta tag (line 5) could include
user-scalable=nofor a more app-like feel, but that's purely aesthetic preference.tests/fiber/reconciliation.test.ts (5)
23-54: Single child reconciliation tests hit the fundamentals!Mount case (lines 23-34): new fiber gets PLACEMENT tag - correct!
Update case (lines 36-54): same type and key means fiber reuse via alternate - CORRECT! This is the optimization that makes React fast. Line 53 validates
parent.child?.alternate === currentChildwhich proves the fiber was reused, not recreated.
159-186: Reordering test is THE money test for reconciliation!This test (lines 159-186) is crucial. It validates that when you have:
- Old order: a, b, c
- New order: c, a, b
The reconciler:
- Finds all three fibers by key
- Reuses them (UPDATE not PLACEMENT)
- Reorders them correctly
This is what separates O(n) reconciliation from O(n²) or O(n³). If you were recreating fibers instead of reusing them, keys would be pointless.
Lines 179-181 validate the new order is correct.
Lines 184-185 validate all fibers were reused (UPDATE effect).Perfect test!
351-362: Duplicate key test is interesting!Line 351-362: multiple children with the same key. This is a user error but the system shouldn't crash. The test just validates that both children are created (line 361).
React would warn about this in dev mode. You might want to consider adding a dev-mode warning when duplicate keys are detected. But for now, handling it gracefully is good enough.
477-499: Large list test validates O(n) algorithm!Creating and reconciling 1000 children (lines 477-499) is important for proving your algorithm scales linearly. If reconciliation were O(n²), this test would be noticeably slow or timeout.
The test counts children in a while loop (lines 492-497) to validate all 1000 were created. This is deterministic and much better than timing assertions.
Previous review complaints about timing were fixed here too!
570-603: Complex reordering test (reverse 10 items) is SPICY!Lines 570-603: create 10 fibers in order 0-9, then reverse to 9-0.
This tests the worst-case reordering scenario. Every fiber moved to the opposite end. If your algorithm is naive, this could cause O(n²) behavior or create unnecessary deletions/placements.
Lines 597-602 validate that ALL fibers were reused (UPDATE effect), not recreated. This proves the key-based diffing works even in the worst case.
Excellent test!
docs/02-work-loop.md (4)
1-141: Documentation is SOLID - this is how you doc architecture!The conversational tone works really well here. Line 5: "Think of it as the main game loop in a video game" - perfect analogy! Makes the concept immediately graspable.
The code examples are helpful and appear to match the actual implementation. The progression from entry point (scheduleUpdateOnFiber) to root discovery to work loop is logical and easy to follow.
Line 81: explaining "sync" means synchronous without yielding, and hinting at future "performConcurrentWorkOnRoot" - good foreshadowing!
182-231: completeUnitOfWork explanation is MONEY!The explanation of effect list building (lines 197-218) is crucial. This is the optimization that makes commit phase O(changes) instead of O(tree-size).
As you bubble up, you append child effects to parent effects, building a linked list. By the time you reach root, you have a complete list of everything that changed.
This is one of the key insights of the fiber architecture and you explained it clearly.
242-263: Tree traversal diagram is CLUTCH!A / \ B C / \ D E Order: 1. beginWork(A) -> returns B 2. beginWork(B) -> returns D ...This ASCII diagram makes the depth-first traversal concrete. Visual learners (like me) appreciate this. More code should have diagrams!
321-377: Wrapping up the doc with practical considerations - nice!The "Memory and Performance" section (lines 321-334) hits the key points:
- No recursion = no stack overflow
- Minimal allocations via alternate pointer reuse
- Effect list optimization
- Early bailout opportunities
The "Integration Points" (lines 335-348) show how work loop coordinates with the rest of the system. This context is valuable for understanding the bigger picture.
Debugging tips (lines 349-366) are actually useful! Most docs skip the "how to debug when it breaks" part.
Overall this is high-quality documentation. Clear, comprehensive, with good examples and diagrams.
| if (typeof type === "string") { | ||
| // Check if it's TEXT_ELEMENT (also a string) | ||
| if (type === TEXT_ELEMENT) { | ||
| completeHostText(current, workInProgress); | ||
| return; | ||
| } | ||
| // Regular host component (div, span, etc) | ||
| completeHostComponent(current, workInProgress); | ||
| } else if (typeof type === "symbol") { | ||
| // Symbol types: FRAGMENT or PORTAL | ||
| if (type === FRAGMENT) { | ||
| completeFragment(current, workInProgress); | ||
| } else if (type === PORTAL) { | ||
| completePortal(current, workInProgress); | ||
| } | ||
| } else if (typeof type === "function") { | ||
| // Functional component (no DOM node) | ||
| completeFunctionComponent(current, workInProgress); | ||
| } else if (type === null) { | ||
| // Root fiber (no DOM node) | ||
| completeHostRoot(current, workInProgress); | ||
| } |
There was a problem hiding this comment.
TEXT fibers are straight up getting skipped
Yo, this dispatch is throwing TEXT_ELEMENT under the bus. TEXT_ELEMENT is a Symbol.for("mini.react.text") in src/core/types, which means every text fiber shows up here with typeof type === "symbol". Because we only branch on "string" first and we don’t have a symbol-path case for TEXT_ELEMENT, completeHostText never runs. We bail out before building the text node, so a simple render(<div>hi</div>) renders <div></div> — zero text, zero chill. It gets worse: even if you patched the dispatch, appendAllChildren only appends string-typed fibers, so the text node would still never hit the DOM.
Fix: short-circuit on type === TEXT_ELEMENT before the string branch and let appendAllChildren append TEXT_ELEMENT fibers as well.
@@
-export function completeWork(
- current: Fiber | null,
- workInProgress: Fiber,
-): void {
- const { type } = workInProgress;
-
- if (typeof type === "string") {
- // Check if it's TEXT_ELEMENT (also a string)
- if (type === TEXT_ELEMENT) {
- completeHostText(current, workInProgress);
- return;
- }
+export function completeWork(
+ current: Fiber | null,
+ workInProgress: Fiber,
+): void {
+ const { type } = workInProgress;
+
+ if (type === TEXT_ELEMENT) {
+ completeHostText(current, workInProgress);
+ return;
+ }
+
+ if (typeof type === "string") {
// Regular host component (div, span, etc)
completeHostComponent(current, workInProgress);
@@
- if (node.stateNode !== null && typeof node.type === "string") {
- // Host component or TEXT_ELEMENT - append its DOM node
+ if (
+ node.stateNode !== null &&
+ (typeof node.type === "string" || node.type === TEXT_ELEMENT)
+ ) {
+ // Host component or text fiber - append its DOM node
parent.appendChild(node.stateNode as Node);Without this, every keyed reorder involving text, every text update, literally all text rendering is broken. Gotta land this fix before shipping.
Also applies to: 352-387
There was a problem hiding this comment.
Actionable comments posted: 29
♻️ Duplicate comments (1)
tests/fiber/completeWork.test.ts (1)
98-118: This test is validating implementation details, not behavior.Lines 98-118: You're checking that
parentDOM.children[0] === childDOM. This is testing that completeWork physically appends the DOM node, which is an implementation detail.What if tomorrow you decide to use DocumentFragment for batching, or implement a different append strategy? This test breaks even though the BEHAVIOR (child is under parent in DOM) is the same.
Test the behavior, not the implementation:
-const parentDOM = parent.stateNode as HTMLElement; -const childDOM = child.stateNode as HTMLElement; - -expect(parentDOM.children.length).toBe(1); -expect(parentDOM.children[0]).toBe(childDOM); +const parentDOM = parent.stateNode as HTMLElement; +const childDOM = child.stateNode as HTMLElement; + +expect(parentDOM.children.length).toBe(1); +expect(parentDOM.contains(childDOM)).toBe(true); +expect(childDOM.parentElement).toBe(parentDOM);
contains()andparentElementcheck the DOM relationship without assuming HOW it was established. More resilient to refactoring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
docs/README.md(1 hunks)example/src/server.ts(1 hunks)src/fiber/beginWork.ts(1 hunks)src/fiber/fiberCreation.ts(1 hunks)src/fiber/fiberFlags.ts(1 hunks)src/fiber/fiberRoot.ts(1 hunks)src/fiber/index.ts(1 hunks)src/fiber/reconcileChildren.ts(1 hunks)src/fiber/types.ts(1 hunks)tests/MiniReact.falsyValues.test.ts(1 hunks)tests/fiber/beginWork.test.ts(1 hunks)tests/fiber/completeWork.test.ts(1 hunks)tests/fiber/workLoop.test.ts(1 hunks)
🧰 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 (9)
tests/fiber/workLoop.test.ts (4)
src/fiber/fiberRoot.ts (1)
createFiberRoot(23-59)src/fiber/fiberCreation.ts (1)
createFiber(25-88)src/fiber/workLoop.ts (2)
scheduleUpdateOnFiber(39-46)getCurrentFiber(282-284)src/MiniReact.ts (3)
AnyMiniReactElement(8-8)FRAGMENT(133-133)PORTAL(133-133)
src/fiber/beginWork.ts (5)
src/fiber/types.ts (1)
Fiber(164-364)src/fiber/reconcileChildren.ts (1)
reconcileChildren(83-102)src/fiber/fiberCreation.ts (1)
cloneChildFibers(177-213)src/fiber/fiberHooks.ts (1)
setCurrentRenderingFiber(34-40)src/fiber/fiberFlags.ts (1)
Deletion(36-36)
tests/fiber/beginWork.test.ts (3)
src/fiber/fiberCreation.ts (2)
createFiber(25-88)createWorkInProgress(105-164)src/fiber/beginWork.ts (1)
beginWork(45-82)src/fiber/fiberRoot.ts (1)
createFiberRoot(23-59)
tests/fiber/completeWork.test.ts (6)
src/fiber/fiberCreation.ts (2)
createFiber(25-88)createWorkInProgress(105-164)src/fiber/completeWork.ts (1)
completeWork(39-67)src/fiber/fiberFlags.ts (2)
Placement(22-22)UpdateEffect(28-28)src/fiber/fiberRoot.ts (1)
createFiberRoot(23-59)src/fiber/beginWork.ts (1)
beginWork(45-82)src/fiber/types.ts (1)
Fiber(164-364)
src/fiber/reconcileChildren.ts (4)
src/MiniReact.ts (2)
AnyMiniReactElement(8-8)TEXT_ELEMENT(133-133)src/fiber/types.ts (2)
Fiber(164-364)isSameElementType(544-549)src/fiber/fiberCreation.ts (4)
createFiberFromElement(223-273)getElementKey(389-407)getElementType(443-465)createWorkInProgress(105-164)src/fiber/fiberFlags.ts (4)
Placement(22-22)hasEffectTag(95-97)Deletion(36-36)UpdateEffect(28-28)
src/fiber/types.ts (2)
src/MiniReact.ts (5)
AnyMiniReactElement(8-8)ElementType(11-11)TEXT_ELEMENT(133-133)FRAGMENT(133-133)PORTAL(133-133)src/hooks/types.ts (1)
StateOrEffectHook(62-69)
src/fiber/fiberRoot.ts (2)
src/fiber/types.ts (3)
FiberRoot(378-429)Fiber(164-364)FIBER_BRAND(18-18)src/fiber/fiberFlags.ts (1)
NoLanes(45-45)
src/fiber/fiberCreation.ts (3)
src/MiniReact.ts (4)
ElementType(11-11)TEXT_ELEMENT(133-133)FRAGMENT(133-133)PORTAL(133-133)src/fiber/types.ts (3)
Props(50-50)Fiber(164-364)FIBER_BRAND(18-18)src/fiber/fiberFlags.ts (2)
NoEffect(16-16)NoLanes(45-45)
src/fiber/fiberFlags.ts (1)
src/fiber/types.ts (3)
EffectTag(57-57)Update(77-92)Lanes(64-64)
🪛 ast-grep (0.39.7)
src/fiber/fiberRoot.ts
[warning] 198-198: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: container.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 LanguageTool
docs/README.md
[style] ~182-~182: This wording could be more concise.
Context: ... performance: O(n) Reconciliation: Linear in number of children Effect Lists: Only pro...
(ADJECTIVE_IN_ATTRIBUTE)
🔇 Additional comments (7)
docs/README.md (3)
17-19: All referenced docs exist—no issues here.The verification confirms all five documentation files are present in the
docsdirectory. Every link in the README.md is pointing to an actual file, so there's nothing to fix. The documentation structure is solid. Ship it.
238-240: Minor punctuation: Sentence fragmentation on line 240.Line 240 reads: "...the debugger. Set a breakpoint..." — you've got a period between two closely related independent clauses. Use a semicolon or split it:
-The best way to understand this code is to step through it with a debugger. Set a breakpoint in `performUnitOfWork` and watch the tree get built. +The best way to understand this code is to step through it with a debugger; set a breakpoint in `performUnitOfWork` and watch the tree get built.Or keep it as two sentences but make the second one less command-y. Super minor, but it reads cleaner with a semicolon.
Likely an incorrect or invalid review comment.
212-216: Testing section assumes Bun but PR context suggests flexibility.Line 212–216 states "Run tests with:
bun test". The PR objectives mention this is an ElysiaJS + Bun example, but if the project supports multiple runtimes (npm, pnpm, etc.), consider adding alternatives:Run tests with: \`\`\`bash +npm test +# or with Bun: bun test \`\`\`This makes it clear which is the "normal" path vs. which is Bun-specific, if that's the case.
⛔ Skipped due to 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.src/fiber/fiberCreation.ts (2)
177-213: cloneChildFibers is solid, but the comment is MORE important than the code.Yo, the comment at lines 168-172 is CRUCIAL:
"This is critical for memo bailout to maintain the double-buffering invariant. If we only clone the first child, sibling pointers still point to the current tree, which causes the work loop to mutate the live tree instead of the WIP tree."
THIS is the bug that took React like 3 years to fully iron out. I'm glad you documented it, but this deserves even more emphasis. This function is the ONLY thing preventing silent tree corruption during bailouts.
The implementation is correct (cloning entire sibling chain, setting return pointers, null-terminating). But add a stronger warning:
/** * Clone the entire child list (all siblings) into the work-in-progress tree * + * ⚠️ CRITICAL: This function MUST clone ALL siblings, not just the first child. + * Failing to clone the complete sibling chain will cause the work loop to mutate + * the committed tree during traversal, corrupting the double-buffer invariant. + * This is a common source of subtle bugs in fiber reconcilers. + * * This is critical for memo bailout to maintain the double-buffering invariant.Comments that prevent architectural disasters deserve the shouting treatment, chat.
105-164: The WIP reuse pattern is properly secured—no tree corruption risk here.The code already implements the exact pattern React uses. After searching through all bailout paths:
- Memo bailout (line 133 of beginWork.ts) — ✓ Calls
cloneChildFibers(current, workInProgress)correctly- Error handling — Errors are caught and rethrown (workLoop.ts:185-188, 228-231), causing the entire WIP tree to be discarded, which prevents any inconsistent state
- Normal reconciliation — All other paths properly call
reconcileChildren, which handles the WIP state throughcreateWorkInProgressThe
reconcileSingleElementfunction (line 216 in reconcileChildren.ts) is the only place wherecreateWorkInProgressgets invoked during diffing, and it's followed by proper setup. When beginWork processes the returned fiber, if the memo bailout fires,cloneChildFibersis already there waiting. If an error occurs, the WIP tree gets thrown away. There's no window where you've got a half-baked WIP tree pointing into the current tree.This is solid. The double-buffering invariant is maintained.
src/fiber/beginWork.ts (1)
238-262: The suggested container comparison approach doesn't match the code architecture.Here's what I found, and honestly, this is a classic case of "right problem, completely wrong solution":
The identity comparison IS there (
oldContainer !== newContainer), comparing HTMLElement objects directly - that part the reviewer nailed.BUT — the suggested fix about comparing
.containerInfois dead wrong. Look at the code flow:
beginWork.tsworks with rawHTMLElementreferences frompendingProps.targetContainer- Those elements don't have a
.containerInfoproperty.containerInfoonly exists instateNode(the wrapper object created incompleteWork.ts)- They're in different contexts entirely — this is architectural confusion
In practice, this isn't actually a problem:
- Tests show usage patterns where
document.createElement()ordocument.getElementById()are passed- Both return stable references (same object = same DOM node)
- The identity comparison works fine for real-world usage
The real architectural question (which the reviewer should have asked):
- Should portal container changes use strict identity or compare actual DOM node values?
- If we want to be pedantic about it: compare
oldContainer === newContainerdirectly (not object identity) or normalize references upfrontThe review comment conflates two different architectural layers. The concern about expensive remounts is theoretically valid but the diagnosis is wrong.
Likely an incorrect or invalid review comment.
tests/fiber/beginWork.test.ts (1)
654-673: This review comment is based on incorrect assumptions about the implementation.The
normalizeChildrenfunction already handles all the edge cases you're concerned about:
- Nested arrays: Handled via explicit recursive call
if (Array.isArray(child)) { result.push(...normalizeChildren(child)); continue; }- Empty arrays: Correctly returns
[]after loop iteration yields nothing- Nested nulls: Recursion combined with upfront filtering
if (child === null || child === undefined || typeof child === "boolean") { continue; }catches them at every level- Fragments: Handled at the fiber creation level (not in
normalizeChildren), wherecreateFiberFromFragmentunwraps Fragment elementsThe implementation is architecturally solid. The test coverage at lines 654-673 is simplistic but doesn't indicate bugs—the actual reconciliation logic is correct.
If you want to strengthen test confidence, yeah, add explicit cases for
[[], span],[[null, span]], etc. But that's test comprehensiveness, not a reconciliation bug.Likely an incorrect or invalid review comment.
|
|
||
| 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. | ||
|
|
||
| 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. |
There was a problem hiding this comment.
🧩 Analysis chain
Test count is inconsistent with PR objectives.
Line 9 claims 539 passing tests, but the PR objectives state 484 passing tests reported. Which is the source of truth here? This mismatch kills credibility in the docs — docs need to match reality.
Before we ship this, we need to nail down the actual test count:
Once you run this, update line 9 with the actual count. Use the real number — no cap.
🏁 Script executed:
#!/bin/bash
# Find the actual test count by checking test files and running them
cd "$(git rev-parse --show-toplevel)"
# Count test files
echo "=== Test Files ==="
fd -e test.ts -e test.tsx -e spec.ts | wc -l
# Try to find test count from package.json or test output
echo "=== Checking package.json for test config ==="
jq '.scripts.test' package.json 2>/dev/null || echo "No test script found"
# Look for any CI config that might report test count
echo "=== GitHub Actions workflow ==="
find .github/workflows -name "*.yml" -o -name "*.yaml" | head -3Length of output: 165
Alright, we found 27 test files, and the test runner is bun test. But here's the thing — finding 27 files doesn't tell us jack about how many tests actually live in those files. One file could have 2 tests, another could have 50. This is sloppy.
We need to check the CI workflow and see what the actual test count is. Let me dig into that workflow file and also poke around for any test output or metadata.
🏁 Script executed:
#!/bin/bash
cd "$(git rev-parse --show-toplevel)"
echo "=== CI Workflow File ==="
cat .github/workflows/ci.yml | head -60
echo -e "\n=== Looking for test report/output files ==="
find . -name "*test*report*" -o -name "*coverage*" -o -name ".test-results*" 2>/dev/null | head -10
echo -e "\n=== Check package.json for test metadata ==="
cat package.json | jq '.devDependencies, .scripts' 2>/dev/null
echo -e "\n=== Sample test file to understand structure ==="
fd -e test.ts -e test.tsx 2>/dev/null | head -1 | xargs head -30Length of output: 2966
Okay, so the CI doesn't actually report test counts — it just yells into the void with bun test. Classic. No metadata, no JSON reports. We gotta count this ourselves.
Let me search for where these magic numbers 539 and 484 are actually coming from, and then count the actual test cases.
🏁 Script executed:
#!/bin/bash
cd "$(git rev-parse --show-toplevel)"
echo "=== Search for 539 in repo ==="
rg "539" --type-list | head -20
echo -e "\n=== Search for 484 in repo ==="
rg "484" --type-list | head -20
echo -e "\n=== Count all test() declarations ==="
rg -c 'test\s*\(' --type ts | awk -F: '{sum += $2} END {print "Total test() calls: " sum}'
echo -e "\n=== Count all describe() declarations ==="
rg -c 'describe\s*\(' --type ts | awk -F: '{sum += $2} END {print "Total describe() calls: " sum}'
echo -e "\n=== Count expect() calls (assertions) ==="
rg -c 'expect\s*\(' --type ts | awk -F: '{sum += $2} END {print "Total expect() calls: " sum}'
echo -e "\n=== Check README.md for where 539 is mentioned ==="
rg "539|484" docs/README.md -C 2Length of output: 1610
Update the test count in docs — it's completely wrong.
Bro, the README is straight-up lying. You're claiming 539 tests but there are only 525 test() declarations in the codebase. That's a 14-test gap. This isn't some philosophical debate — we can literally count them.
The docs say it twice:
- Line 9: "539 passing tests"
- Later section: "The test suite has 539 passing tests"
Both need to say 525, not 539. You can't just make up numbers here — docs have to match reality or nobody trusts anything else you write. This is architecture credibility 101.
Fix both instances in docs/README.md and use the real number. Done.
🤖 Prompt for AI Agents
In docs/README.md around line 9 (and the later duplicate mention), the README
incorrectly states "539 passing tests" — update the text at line 9 and the other
section where it repeats to read "525 passing tests" instead; perform the
replacement for both occurrences in README.md and verify no other "539"
instances remain in that file.
| ### Code Organization | ||
|
|
||
| ```text | ||
| src/ | ||
| ├── fiber/ | ||
| │ ├── workLoop.ts # Main rendering loop | ||
| │ ├── beginWork.ts # Process each fiber | ||
| │ ├── completeWork.ts # Finalize each fiber | ||
| │ ├── commitWork.ts # Apply DOM changes | ||
| │ ├── reconcileChildren.ts # Diffing algorithm | ||
| │ ├── fiberHooks.ts # Hook implementations | ||
| │ └── types.ts # Fiber type definitions | ||
| ├── events/ | ||
| │ ├── eventSystem.ts # Event delegation | ||
| │ └── types.ts # Event type definitions | ||
| ├── hooks/ | ||
| │ └── fiberHooksImpl.ts # Public hook API | ||
| ├── context/ | ||
| │ └── index.ts # Context implementation | ||
| └── portals/ | ||
| └── index.ts # Portal implementation | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
Code organization tree is helpful but verify paths match reality.
The code organization section lays out a clean tree structure. Make sure this matches what's actually in src/fiber/ and src/ at the time this PR lands.
If the actual structure differs from what's documented (lines 116–134), update the tree to match reality. Docs lying about file structure is super confusing for new contributors.
🏁 Script executed:
#!/bin/bash
# Verify code organization matches the documented structure
echo "=== Actual file structure in src/ ==="
find src -type f -name "*.ts" -o -name "*.tsx" | sortLength of output: 823
Update the README file tree—it's missing entire directories and ~35 files.
The documented structure is severely out of sync with reality. The docs only show 7 files in fiber/ when there are 12. Missing entire top-level directories like core/, dom-renderer/, fragments/, jsx-runtime/, performance/, and reconciler/. Also missing index.ts and types.ts files in several documented modules.
This creates friction for new contributors trying to navigate the codebase. The tree needs to match what's actually there. Here's what's missing:
- Root level:
MiniReact.ts,jsx-dev-runtime.ts,jsx-runtime.ts,jsx.d.ts - New directories:
core/,dom-renderer/,fragments/,jsx-runtime/,performance/,reconciler/ - Fiber directory:
domOperations.ts,fiberCreation.ts,fiberFlags.ts,fiberRoot.ts,index.ts - Partial updates needed:
events/(addindex.ts),context/(addtypes.ts),portals/(addtypes.ts),hooks/(addindex.ts,types.ts)
Update lines 116–134 to reflect the actual structure or document why these files/directories are intentionally omitted from the user-facing guide.
🤖 Prompt for AI Agents
In docs/README.md around lines 113 to 134, the displayed project tree is
outdated and missing many top-level files, new directories, and several files
inside fiber/events/context/portals/hooks; update the README tree to exactly
reflect the repository layout by adding the root files (MiniReact.ts,
jsx-dev-runtime.ts, jsx-runtime.ts, jsx.d.ts), the new directories (core,
dom-renderer, fragments, jsx-runtime, performance, reconciler), the missing
fiber files (domOperations.ts, fiberCreation.ts, fiberFlags.ts, fiberRoot.ts,
index.ts), add index.ts to events, add types.ts to context and portals, and add
index.ts and types.ts to hooks (or, if omission is intentional, add a short note
in lines 116–134 explaining why those files/directories are excluded from the
guide); ensure the tree formatting and file names match the actual repo
structure and update line ranges 116–134 accordingly.
|
|
||
| The architecture is designed for performance: | ||
|
|
||
| **O(n) Reconciliation**: Linear in number of children |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tighten the wording — "O(n)" already implies linear.
Line 182: **O(n) Reconciliation**: Linear in number of children is doing double-work. O(n) is linear. Pick one.
-**O(n) Reconciliation**: Linear in number of children
+**O(n) Reconciliation**: Optimal diffing complexityOr if you want to be more specific:
-**O(n) Reconciliation**: Linear in number of children
+**O(n) Reconciliation**: Scales linearly with sibling countThis fixes the static analysis hint too. Keep it tight.
📝 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.
| **O(n) Reconciliation**: Linear in number of children | |
| **O(n) Reconciliation**: Optimal diffing complexity |
🧰 Tools
🪛 LanguageTool
[style] ~182-~182: This wording could be more concise.
Context: ... performance: O(n) Reconciliation: Linear in number of children Effect Lists: Only pro...
(ADJECTIVE_IN_ATTRIBUTE)
🤖 Prompt for AI Agents
In docs/README.md around line 182, the heading "O(n) Reconciliation: Linear in
number of children" is redundant; replace it with a single, tighter phrasing
such as either "O(n) Reconciliation" or "Reconciliation — O(n) in number of
children" (or "Reconciliation: proportional to number of children") so the
complexity notation and description aren’t duplicated, and update the line
accordingly.
| ## Further Reading | ||
|
|
||
| Want to go deeper? | ||
|
|
||
| **React Fiber Architecture**: The original React Fiber design doc | ||
|
|
||
| **React Reconciliation**: Official React docs on reconciliation | ||
|
|
||
| **Inside Fiber**: Deep dive talks by React team members | ||
|
|
||
| **Build Your Own React**: Tutorials building React from scratch | ||
|
|
There was a problem hiding this comment.
"Further Reading" section needs actual links or should be removed.
Lines 228–239 reference:
- "React Fiber Architecture: The original React Fiber design doc"
- "React Reconciliation: Official React docs on reconciliation"
- "Inside Fiber: Deep dive talks by React team members"
- "Build Your Own React: Tutorials building React from scratch"
These look like they should be links but aren't formatted as markdown links. Either add actual URLs or remove this section entirely. Dead references in docs are confusing.
If these are meant to be real links, use markdown syntax:
-**React Fiber Architecture**: The original React Fiber design doc
+**[React Fiber Architecture](https://react.dev/learn/understanding-react)**: The original React Fiber design docOr if you want to keep it simple and these aren't critical, just remove the section. Users can Google if they need more info.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/README.md around lines 228 to 239, the "Further Reading" entries are
plain text and must be converted to real links or the section removed; update
each bullet to use markdown link syntax with appropriate URLs (for example:
React Fiber Architecture -> https://github.com/acdlite/react-fiber-architecture,
React Reconciliation -> https://reactjs.org/docs/reconciliation.html, Inside
Fiber talks -> https://www.youtube.com/watch?v=ZCuYPiUIONs, Build Your Own React
-> https://pomb.us/build-your-own-react/) or, if you prefer not to maintain
external links, delete the entire section. Ensure links open in new tabs by
using standard markdown link formatting and keep the list concise.
| const app = new Elysia() | ||
| .get("/app.js", () => { | ||
| const file = Bun.file("./public/app.js"); | ||
| return new Response(file, { | ||
| headers: { | ||
| "Content-Type": "application/javascript; charset=utf-8", | ||
| "Cache-Control": "public, max-age=3600", | ||
| }, | ||
| }); | ||
| }) | ||
| .get("/", () => { | ||
| const file = Bun.file("./src/templates/index.html"); | ||
| const headers: Record<string, string> = { | ||
| "Content-Type": "text/html", | ||
| }; | ||
|
|
||
| // Add caching in production | ||
| if (process.env.NODE_ENV === "production") { | ||
| headers["Cache-Control"] = "public, max-age=3600"; | ||
| } | ||
|
|
||
| return new Response(file, { headers }); | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing some production-ready concerns (but it's an example so... maybe fine?)
Look, this is an example app, so I'm not gonna go full enterprise architect on you. But since you're already thinking about production (line 22, the NODE_ENV check), let me drop some knowledge:
-
No security headers - You're serving content but not setting security headers like
X-Content-Type-Options: nosniff,X-Frame-Options: DENY, etc. These are basically free security wins. -
No request logging - In production, you'd want to know what requests are hitting your server. Elysia has middleware for this.
-
No global error handler - If something throws, what happens? Does Elysia catch it? Does the server crash? Unknown.
Again, for an example app showcasing a fiber renderer, this is probably fine. But if this pattern gets copied to production code, these things matter.
If you want to make this more production-ready:
import { Elysia } from "elysia";
import { join } from "path";
const PROJECT_ROOT = join(import.meta.dir, "..");
const PORT = Number(process.env.PORT) || 3000;
const app = new Elysia()
// Security headers middleware
.onAfterHandle(({ response, set }) => {
set.headers = {
...set.headers,
"X-Content-Type-Options": "nosniff",
"X-Frame-Options": "DENY",
"X-XSS-Protection": "1; mode=block",
};
})
// Error handling
.onError(({ code, error }) => {
console.error(`Error [${code}]:`, error);
return new Response("Internal Server Error", { status: 500 });
})
.get("/app.js", () => {
// ... with fixed paths ...
})
.listen(PORT);But again - example app. Your call.
🤖 Prompt for AI Agents
In example/src/server.ts around lines 5 to 27, the example routes lack
production-safe concerns: add a middleware to inject common security headers
(e.g. X-Content-Type-Options: nosniff, X-Frame-Options: DENY, X-XSS-Protection:
1; mode=block, and relevant CSP/Referrer-Policy as needed), add a
request-logging middleware to record incoming requests (method, path, status,
timing) for observability, and add a global error handler (.onError or
equivalent) that logs the error and returns a 500 response; keep the existing
caching logic for production but ensure middleware runs for every route and
imports/config (PORT, PROJECT_ROOT) are added if needed for listening in
production.
| // After processing, root should have finishedWork | ||
| // Phase 3: finishedWork is cleared after commit | ||
| expect(root.finishedWork).toBeNull(); | ||
| }); | ||
|
|
||
| test("should set finishedWork after render", () => { | ||
| const root = createFiberRoot(container); | ||
| const rootFiber = root.current; | ||
|
|
||
| rootFiber.pendingProps = { children: [] }; | ||
|
|
||
| scheduleUpdateOnFiber(rootFiber); | ||
|
|
||
| // In Phase 2, finishedWork is set after render phase completes | ||
| // It will be cleared in Phase 4 (commit phase) after being committed | ||
| // Phase 3: finishedWork is cleared after commit | ||
| expect(root.finishedWork).toBeNull(); | ||
| expect(root.current.alternate).toBe(rootFiber); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
These comments about "Phase 3" are confusing AF.
Lines 62, 63, 77:
// Phase 3: finishedWork is cleared after commit
expect(root.finishedWork).toBeNull();What is "Phase 3"? Is that some internal development phase? A reconciliation phase? The comment doesn't help me understand WHY finishedWork should be null.
The test name says "should set finishedWork after render" but then expects it to be null because it was cleared after commit. That's... testing the opposite of the name?
Either:
- Fix the test name to match what you're actually testing:
-test("should set finishedWork after render", () => {
+test("should clear finishedWork after commit completes", () => {- Or update comments to be clearer:
-// Phase 3: finishedWork is cleared after commit
+// finishedWork is set during render but cleared after commit completesAnd remove "Phase 3" unless you've documented what these phases are elsewhere. If you're referring to React's actual reconciliation phases (render/commit), say that explicitly.
| // Check that effects were collected | ||
| const finishedWork = root.current; // Phase 3: check committed tree | ||
| expect(finishedWork).not.toBeNull(); | ||
|
|
||
| // Root should have effects from children | ||
| // Either firstEffect or child should have effects | ||
| const hasEffects = | ||
| finishedWork?.firstEffect !== null || finishedWork?.child !== null; | ||
| expect(hasEffects).toBe(true); | ||
| }); |
There was a problem hiding this comment.
This test is checking the wrong thing.
Lines 233-242:
const finishedWork = root.current; // Phase 3: check committed tree
expect(finishedWork).not.toBeNull();
// Root should have effects from children
// Either firstEffect or child should have effects
const hasEffects =
finishedWork?.firstEffect !== null || finishedWork?.child !== null;
expect(hasEffects).toBe(true);Bruh, you're checking "firstEffect OR child" exists? That's like saying "the pizza has pepperoni OR it exists" - of course it's true if the fiber has ANY child, regardless of effects!
You should be checking that effects were actually collected:
-const hasEffects =
- finishedWork?.firstEffect !== null || finishedWork?.child !== null;
-expect(hasEffects).toBe(true);
+// Root should have collected placement effects from children
+expect(
+ finishedWork?.firstEffect !== null ||
+ (finishedWork?.child && finishedWork.child.effectTag !== 0)
+).toBe(true);Or better yet, just check what you actually care about:
expect(finishedWork?.firstEffect).not.toBeNull();Be specific about what you're testing, chat.
🤖 Prompt for AI Agents
In tests/fiber/workLoop.test.ts around lines 233-242, the test currently checks
a boolean that ORs finishedWork.firstEffect with finishedWork.child which
falsely passes when any child exists; update the assertion to specifically
verify that effects were collected by asserting finishedWork is not null and
that finishedWork.firstEffect is not null (i.e., remove the OR-with-child check
and replace it with expect(finishedWork?.firstEffect).not.toBeNull()).
| test("should handle large trees efficiently", () => { | ||
| const root = createFiberRoot(container); | ||
| const rootFiber = root.current; | ||
|
|
||
| // Create a wide tree (100 siblings) | ||
| const children = []; | ||
| for (let i = 0; i < 100; i++) { | ||
| children.push({ type: "div", props: { key: i, children: [] } }); | ||
| } | ||
|
|
||
| rootFiber.pendingProps = { children }; | ||
|
|
||
| scheduleUpdateOnFiber(rootFiber); | ||
|
|
||
| // Verify tree was constructed correctly | ||
| expect(root.current).not.toBeNull(); | ||
|
|
||
| // Count siblings to verify all 100 children were created | ||
| let count = 0; | ||
| let child = root.current.child; | ||
| while (child) { | ||
| count++; | ||
| child = child.sibling; | ||
| } | ||
| expect(count).toBe(100); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test name is lying about what it validates.
Line 508: test("should handle large trees efficiently")
But then the test just creates 100 children and checks they were all created (lines 526-532). There's no efficiency validation - no timing, no memory checks, nothing performance-related. You literally just count to 100.
After past review removed timing assertions (good call), the test name became misleading.
-test("should handle large trees efficiently", () => {
+test("should handle wide trees with many siblings", () => {And same for line 535:
-test("should handle deep trees efficiently", () => {
+test("should handle deeply nested trees", () => {Don't claim "efficiently" unless you're measuring something. These are correctness tests, not performance tests. Own it, chat.
🤖 Prompt for AI Agents
In tests/fiber/workLoop.test.ts around lines 508–533 the test title "should
handle large trees efficiently" is misleading because the test only asserts that
100 children were created and does not measure performance; rename the test to
something like "should create wide trees with 100 children" (and similarly
update the other test around line 535), or alternatively add explicit
timing/memory assertions if you really want to validate efficiency—make the
change so the test name accurately reflects whether it is a correctness check or
a performance test.
| /** | ||
| * Comprehensive tests for falsy value rendering | ||
| * | ||
| * Tests cover: | ||
| * - Rendering 0, empty string, false, null, undefined | ||
| * - Conditional rendering patterns | ||
| * - Arrays with falsy values | ||
| * - Switching between falsy and truthy values | ||
| */ | ||
|
|
||
| import { afterEach, beforeEach, describe, expect, test } from "bun:test"; | ||
| import { render } from "../src/MiniReact"; | ||
|
|
||
| describe("MiniReact.FalsyValues - Comprehensive", () => { | ||
| let container: HTMLElement; | ||
|
|
||
| beforeEach(() => { | ||
| container = document.createElement("div"); | ||
| document.body.appendChild(container); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| if (container?.parentNode) { | ||
| container.parentNode.removeChild(container); | ||
| } | ||
| }); | ||
|
|
||
| describe("Rendering Number 0", () => { | ||
| test("should render 0 as text content", () => { | ||
| render({ type: "div", props: { children: [0] } }, container); | ||
|
|
||
| expect(container.textContent).toBe("0"); | ||
| }); | ||
|
|
||
| test("should render 0 in nested structure", () => { | ||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: [{ type: "span", props: { children: [0] } }], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| expect(container.querySelector("span")?.textContent).toBe("0"); | ||
| }); | ||
|
|
||
| test("should handle switching from 0 to another number", () => { | ||
| render({ type: "div", props: { children: [0] } }, container); | ||
| expect(container.textContent).toBe("0"); | ||
|
|
||
| render({ type: "div", props: { children: [42] } }, container); | ||
| expect(container.textContent).toBe("42"); | ||
| }); | ||
|
|
||
| test("should handle switching from truthy to 0", () => { | ||
| render({ type: "div", props: { children: ["hello"] } }, container); | ||
| expect(container.textContent).toBe("hello"); | ||
|
|
||
| render({ type: "div", props: { children: [0] } }, container); | ||
| expect(container.textContent).toBe("0"); | ||
| }); | ||
|
|
||
| test("should render multiple zeros", () => { | ||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: [0, 0, 0], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| expect(container.textContent).toBe("000"); | ||
| }); | ||
|
|
||
| test("should render 0 in array with other values", () => { | ||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: ["count: ", 0], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| expect(container.textContent).toBe("count: 0"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Rendering Empty String", () => { | ||
| test("should render empty string", () => { | ||
| render({ type: "div", props: { children: [""] } }, container); | ||
|
|
||
| expect(container.textContent).toBe(""); | ||
| expect(container.childNodes.length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| test("should handle switching from empty string to content", () => { | ||
| render({ type: "div", props: { children: [""] } }, container); | ||
| expect(container.textContent).toBe(""); | ||
|
|
||
| render({ type: "div", props: { children: ["hello"] } }, container); | ||
| expect(container.textContent).toBe("hello"); | ||
| }); | ||
|
|
||
| test("should handle switching from content to empty string", () => { | ||
| render({ type: "div", props: { children: ["hello"] } }, container); | ||
| expect(container.textContent).toBe("hello"); | ||
|
|
||
| render({ type: "div", props: { children: [""] } }, container); | ||
| expect(container.textContent).toBe(""); | ||
| }); | ||
|
|
||
| test("should render empty string as prop value", () => { | ||
| render({ type: "input", props: { value: "", children: [] } }, container); | ||
|
|
||
| const input = container.querySelector("input"); | ||
| expect(input?.value).toBe(""); | ||
| }); | ||
|
|
||
| test("should render empty string in array", () => { | ||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: ["start", "", "end"], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| expect(container.textContent).toBe("startend"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Rendering Boolean False", () => { | ||
| test("should not render false as text content", () => { | ||
| render({ type: "div", props: { children: [false] } }, container); | ||
|
|
||
| expect(container.textContent).toBe(""); | ||
| }); | ||
|
|
||
| test("should handle conditional rendering with false", () => { | ||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: [ | ||
| false, | ||
| { type: "span", props: { children: ["visible"] } }, | ||
| ], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| expect(container.textContent).toBe("visible"); | ||
| expect(container.querySelector("span")).not.toBeNull(); | ||
| }); | ||
|
|
||
| test("should handle switching from false to true (both render nothing)", () => { | ||
| render({ type: "div", props: { children: [false] } }, container); | ||
| expect(container.textContent).toBe(""); | ||
|
|
||
| render({ type: "div", props: { children: [true] } }, container); | ||
| // true also doesn't render as text (React behavior) | ||
| expect(container.textContent).toBe(""); | ||
| }); | ||
|
|
||
| test("should use false as prop value", () => { | ||
| render( | ||
| { type: "input", props: { disabled: false, children: [] } }, | ||
| container, | ||
| ); | ||
|
|
||
| const input = container.querySelector("input"); | ||
| expect(input?.disabled).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Rendering Null", () => { | ||
| test("should render nothing for null", () => { | ||
| render(null, container); | ||
|
|
||
| expect(container.textContent).toBe(""); | ||
| expect(container.childNodes.length).toBe(0); | ||
| }); | ||
|
|
||
| test("should handle null in children array", () => { | ||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: ["before", null, "after"], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| expect(container.textContent).toBe("beforeafter"); | ||
| }); | ||
|
|
||
| test("should handle switching from element to null", () => { | ||
| render({ type: "div", props: { children: ["content"] } }, container); | ||
| expect(container.textContent).toBe("content"); | ||
|
|
||
| render(null, container); | ||
| expect(container.textContent).toBe(""); | ||
| expect(container.childNodes.length).toBe(0); | ||
| }); | ||
|
|
||
| test("should handle switching from null to element", () => { | ||
| render(null, container); | ||
| expect(container.textContent).toBe(""); | ||
|
|
||
| render({ type: "div", props: { children: ["content"] } }, container); | ||
| expect(container.textContent).toBe("content"); | ||
| }); | ||
|
|
||
| test("should handle nested null values", () => { | ||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: [{ type: "span", props: { children: [null] } }, "text"], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| expect(container.textContent).toBe("text"); | ||
| expect(container.querySelector("span")).not.toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Rendering Undefined", () => { | ||
| test("should render nothing for undefined", () => { | ||
| render(undefined, container); | ||
|
|
||
| expect(container.textContent).toBe(""); | ||
| expect(container.childNodes.length).toBe(0); | ||
| }); | ||
|
|
||
| test("should handle undefined in children array", () => { | ||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: ["before", undefined, "after"], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| expect(container.textContent).toBe("beforeafter"); | ||
| }); | ||
|
|
||
| test("should handle switching from element to undefined", () => { | ||
| render({ type: "div", props: { children: ["content"] } }, container); | ||
| expect(container.textContent).toBe("content"); | ||
|
|
||
| render(undefined, container); | ||
| expect(container.textContent).toBe(""); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Mixed Falsy Values", () => { | ||
| test("should handle array with multiple falsy types", () => { | ||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: [0, "", false, null, undefined, "text"], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| // 0 and "" render, false/null/undefined don't | ||
| expect(container.textContent).toBe("0text"); | ||
| }); | ||
|
|
||
| test("should handle switching between different falsy values", () => { | ||
| render({ type: "div", props: { children: [0] } }, container); | ||
| expect(container.textContent).toBe("0"); | ||
|
|
||
| render({ type: "div", props: { children: [""] } }, container); | ||
| expect(container.textContent).toBe(""); | ||
|
|
||
| render({ type: "div", props: { children: [false] } }, container); | ||
| expect(container.textContent).toBe(""); | ||
|
|
||
| render({ type: "div", props: { children: [null] } }, container); | ||
| expect(container.textContent).toBe(""); | ||
| }); | ||
|
|
||
| test("should handle elements with falsy props", () => { | ||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| id: "", | ||
| "data-count": 0, | ||
| "data-flag": false, | ||
| children: [], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| const div = container.querySelector("div"); | ||
| expect(div?.getAttribute("id")).toBe(""); | ||
| expect(div?.getAttribute("data-count")).toBe("0"); | ||
| // false removes the attribute (doesn't set it) | ||
| expect(div?.getAttribute("data-flag")).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Conditional Rendering Patterns", () => { | ||
| test("should handle logical AND with falsy left side", () => { | ||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: [ | ||
| false, | ||
| { type: "span", props: { children: ["never shown"] } }, | ||
| ], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| expect(container.querySelector("span")).not.toBeNull(); | ||
| }); | ||
|
|
||
| test("should handle ternary with falsy values", () => { | ||
| // Simulating: condition ? 0 : "fallback" | ||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: [0], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| expect(container.textContent).toBe("0"); | ||
|
|
||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: ["fallback"], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| expect(container.textContent).toBe("fallback"); | ||
| }); | ||
|
|
||
| test("should handle nested conditionals with falsy values", () => { | ||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: [ | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: [ | ||
| false, | ||
| { | ||
| type: "span", | ||
| props: { | ||
| children: [0], | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| expect(container.querySelector("span")?.textContent).toBe("0"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Edge Cases", () => { | ||
| test("should handle NaN", () => { | ||
| render({ type: "div", props: { children: [Number.NaN] } }, container); | ||
|
|
||
| expect(container.textContent).toBe("NaN"); | ||
| }); | ||
|
|
||
| test("should handle -0", () => { | ||
| render({ type: "div", props: { children: [-0] } }, container); | ||
|
|
||
| expect(container.textContent).toBe("0"); | ||
| }); | ||
|
|
||
| test("should handle very small numbers", () => { | ||
| render({ type: "div", props: { children: [0.0000001] } }, container); | ||
|
|
||
| // JavaScript may use scientific notation for very small numbers | ||
| expect(container.textContent).toMatch(/1e-7|0\.0000001/); | ||
| }); | ||
|
|
||
| test("should handle empty array", () => { | ||
| render({ type: "div", props: { children: [] } }, container); | ||
|
|
||
| expect(container.textContent).toBe(""); | ||
| }); | ||
|
|
||
| test("should handle array of only falsy values", () => { | ||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: [false, null, undefined], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| expect(container.textContent).toBe(""); | ||
| }); | ||
|
|
||
| test("should preserve whitespace-only strings", () => { | ||
| render({ type: "div", props: { children: [" "] } }, container); | ||
|
|
||
| expect(container.textContent).toBe(" "); | ||
| }); | ||
|
|
||
| test("should handle newline and tab characters", () => { | ||
| render({ type: "div", props: { children: ["\n\t"] } }, container); | ||
|
|
||
| expect(container.textContent).toBe("\n\t"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
This test suite is CLEAN, chat. Props where props are due.
The organization is chef's kiss:
- Clear describe blocks by value type
- Tests for switching between values (lines 49-63)
- Edge cases properly isolated (lines 398-449)
- Proper setup/teardown
But I'm seeing one missing test case that React actually handles: Symbol values. Check this out:
render(<div>{Symbol('test')}</div>, container)React throws an error for Symbols in children. Your tests cover 0, "", false, null, undefined, NaN, but not Symbols. Since Symbols are falsy-adjacent (can't be rendered) and increasingly common with Symbol.iterator, Symbol.asyncIterator, etc., you should test this behavior.
Add to the Edge Cases section:
test("should handle Symbol values", () => {
const sym = Symbol("test");
// Symbols can't be rendered as children
// Should either skip them or throw a clear error
expect(() =>
render(
{ type: "div", props: { children: [sym] } },
container
)
).toThrow(); // or test your skip behavior if you silently ignore
});This will catch if you accidentally try to String(Symbol()) somewhere, which throws.
🤖 Prompt for AI Agents
In tests/MiniReact.falsyValues.test.ts around lines 398 to 449 (Edge Cases),
there is no test for rendering Symbol children — React throws for Symbol
children and we should cover that; add a test named "should handle Symbol
values" that creates a Symbol("test") and asserts the render call with that
symbol child either throws (expect(() => render(...)).toThrow()) or, if your
implementation intentionally ignores Symbols, asserts that container.textContent
remains "" — implement the assertion that matches your intended behavior and
place the test in the Edge Cases block.
| test("should render 0 in array with other values", () => { | ||
| render( | ||
| { | ||
| type: "div", | ||
| props: { | ||
| children: ["count: ", 0], | ||
| }, | ||
| }, | ||
| container, | ||
| ); | ||
|
|
||
| expect(container.textContent).toBe("count: 0"); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test name doesn't match what it's testing.
Line 79: test("should render 0 in array with other values")
But the test does children: ["count: ", 0] - that's not "in array with other values", that's specifically testing 0 following a string. The name makes it sound like you're testing [1, 0, 2].
More precise name:
-test("should render 0 in array with other values", () => {
+test("should render 0 adjacent to string content", () => {Or if you want to test the "array with other values" case, add:
test("should render 0 in array with other numbers", () => {
render(
{
type: "div",
props: { children: [1, 0, 2] }
},
container
);
expect(container.textContent).toBe("102");
});Minor nitpick but test names are documentation, my dude.
- Update event system cleanup documentation to match implementation - Fix type errors in fiber effect tags tests Remove legacy code (~1,400 lines): - Delete src/reconciler/ (replaced by Fiber architecture) - Delete src/dom-renderer/ (no longer used) - Delete tests/MiniReact.reconciler.test.ts (obsolete) - Delete src/hooks/index.ts.backup (outdated backup) Consolidate duplicated code: - Refactor src/events/eventSystem.ts to import types from types.ts - Merge src/hooks/fiberHooksImpl.ts into index.ts - Remove ~150 lines of duplicate type definitions Result: Cleaner codebase, zero breaking changes, all 520 tests passing
Apply Biome formatting to recently modified files to ensure consistent tab indentation across the codebase.
- Add setup-hooks.sh script to install git hooks - Pre-commit hook automatically formats staged files with Biome - Update README with hook setup instructions - Prevents formatting inconsistencies between commits
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hooks/index.ts (2)
39-88: CompateffectQueueis fine, but this global shim needs a tighter storyThis whole block absolutely screams “legacy shim”. It works, but:
- It’s a single global
effectQueuewith no lifecycle. Tests or multiple roots can leak work across runs unless they manually pokegetEffectQueue()and mutate it.getEffectQueuehands back the live array; any caller can nuke the queue shape.queueMicrotaskis fine for modern runtimes, but you’ve now hard‑wired an environment assumption into the hooks module.I’d strongly consider:
- Moving this into a tiny internal “legacy scheduler” module so the main hooks surface isn’t coupled to this global.
- Exposing a
resetEffectQueueForTests()or similar instead of forcing tests to mutate the array directly.- (Optional) Adding a trivial fallback for environments without
queueMicrotask(e.g.Promise.resolve().then(flushEffects)).All of this keeps the hack where it belongs and makes your future self less miserable when you finally rip this out.
96-152: Yo, setter identity is getting thrashed every render — that's your major issue right hereReact guarantees
useStateanduseReducerreturn stable closures across renders. Your code is torching that contract.Look at line 141–152 in
useState: you're cooking up a brand newsetStateclosure on every single render. Same thing is happening inuseReducer(lines 162–237). Every time the fiber re-renders, you're allocating fresh function objects and returning them. That means:
- Dependency arrays see
setStateas "changed" every render — effects fire unnecessarily.useMemoanduseCallbackthat depend on these get invalidated constantly.- Users get the classic "why is my effect running in an infinite loop?" nightmare.
Fix: Create the dispatch closure once during hook initialization (inside the "initialize hook" branch), store it on the hook object, and return the same reference every render. Make the closure read the current
fiber/queue/reducerfrom a mutable container so it stays fresh without changing identity.This applies to both
useState(line 96–152) anduseReducer(line 162–237).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
README.md(1 hunks)docs/04-event-system.md(1 hunks)example/package.json(1 hunks)scripts/setup-hooks.sh(1 hunks)src/dom-renderer/index.ts(0 hunks)src/events/eventSystem.ts(2 hunks)src/hooks/index.ts(13 hunks)src/reconciler/index.ts(0 hunks)tests/MiniReact.reconciler.test.ts(0 hunks)tests/fiber/effectTags.test.ts(1 hunks)
💤 Files with no reviewable changes (3)
- tests/MiniReact.reconciler.test.ts
- src/dom-renderer/index.ts
- src/reconciler/index.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/hooks/index.ts (4)
src/MiniReact.ts (8)
useState(17-17)UseStateHook(25-25)useEffect(18-18)EffectCallback(29-29)DependencyList(30-30)useRef(20-20)MutableRefObject(32-32)useMemo(21-21)src/hooks/types.ts (7)
UseStateHook(71-74)StateHook(8-13)ReducerHook(30-36)EffectCallback(77-77)DependencyList(78-78)EffectHook(15-22)MutableRefObject(113-115)src/fiber/fiberHooks.ts (7)
getCurrentRenderingFiber(49-51)createUpdateQueue(62-72)processUpdateQueue(151-182)dispatchSetState(110-140)processReducerQueue(236-270)dispatchReducerAction(195-225)areDepsEqual(282-304)src/fiber/types.ts (1)
UpdateQueue(102-139)
🪛 LanguageTool
docs/04-event-system.md
[style] ~51-~51: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...ent` is cached so we can add/remove the exact same function reference. ## Initialization ...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
🔇 Additional comments (8)
example/package.json (2)
7-7: Dev script fix is solid, but let's keep it sharp.The
--watchflag placement looks correct now (immediately afterbun), so that's good — the watcher will actually fire when you touch code. Props for fixing that. Just keep an eye on Bun's behavior here during development cycles.
19-21: Verifymini-reactdependency placement in workspace.The
example/folder no longer listsmini-reactas a dependency. That's only okay if this runs as part of a monorepo workspace wheremini-reactis available as a sibling package. If someone tries to install this folder standalone, it'll explode. Make sure your workspace/package.json setup handles this correctly (or add it here explicitly with a workspace: reference).README.md (1)
117-118: Setup hooks documentation looks clean.Dude, this is solid. You've got the hooks step in exactly the right spot—between dependencies and tests, which is the proper order. The comment is concise and tells you what's happening. Nice.
tests/fiber/effectTags.test.ts (3)
24-63: YOOO, this is actually clean! Bitwise flag testing done right!Testing the actual bit patterns with binary literals (
0b0001) is chef's kiss. This is exactly how you verify your bit flags aren't getting messed up. Love that you're testinghasEffectTagwith single flags, combined flags, and all three flags. This is the foundation that prevents those 3am "why is my reconciliation broken" debugging sessions.No notes here, this slaps.
65-112: completeWork flag preservation tests are solidYo, I appreciate that you're testing BOTH cases here - when Update SHOULD be added (line 66-80) and when it should NOT (line 82-96). Too many people only test the happy path. The fact that you're explicitly checking "props unchanged = no Update flag" shows you understand the reconciliation contract.
The text node change test (98-111) is essential because text nodes are a different code path and people forget to test them.
176-228: Edge case coverage is INSANE (in a good way)Okay, testing bitwise AND NOT (
& ~Placement) for flag removal? Testing toggle operations? Checking multiple flags at once with boolean logic? This is the kind of paranoid testing that saves you when someone refactors the flag system in 6 months and accidentally breaks everything.Line 187-195 testing flag removal with
& ~Placementis especially good because that's the kind of operation that can silently fail if someone changes the flag values.This section is absolutely unhinged coverage and I'm here for it.
src/events/eventSystem.ts (1)
7-14: Centralizing event types + mapping into./typesis the right callThis is the kind of refactor I actually like: one source of truth for
MINI_REACT_EVENT_TO_NATIVE_EVENTand the event types instead of sprinkling them across files. Keeps the event system lean and makes changing mappings/types later not feel like defusing a bomb.src/hooks/index.ts (1)
239-289: Implementation is correct—no changes neededThe
areDepsEqualfunction atsrc/fiber/fiberHooks.ts:282-303perfectly implements React's dependency array semantics:
- Omitted deps (both
undefined): returnsfalse→ effects/memos run every render ✓- Empty array
[]: first call returnsfalse(mount), subsequent calls returntrue(stable) ✓- Non-empty arrays: uses
Object.isper-element comparison exactly like React ✓All three hooks (
useEffect,useMemo,useCallback) wire this up correctly. The semantics are locked down solid.
| cleanup(): void { | ||
| if (this.rootContainer) { | ||
| for (const eventName of this.registeredEvents) { | ||
| this.rootContainer.removeEventListener( | ||
| eventName, | ||
| this.boundHandleDelegatedEvent | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // Remove event listeners from all portal containers | ||
| for (const portalContainer of this.portalContainers) { | ||
| for (const eventName of this.registeredEvents) { | ||
| portalContainer.removeEventListener( | ||
| eventName, | ||
| this.boundHandleDelegatedEvent | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| this.registeredEvents.clear() | ||
| this.portalContainers.clear() | ||
| this.instanceToNode = new WeakMap() | ||
| this.nodeToInstance = new WeakMap() | ||
| this.rootContainer = null | ||
| } |
There was a problem hiding this comment.
Docs cleanup snippet bakes in the same removeEventListener mismatch
The writeup here is great, but the cleanup() example is still showing the “remove without options” pattern:
this.rootContainer.removeEventListener(
eventName,
this.boundHandleDelegatedEvent
)Given listeners are added with getEventOptions(eventName) (capture/passive), the doc should match the fixed implementation and demonstrate passing the same options on removal, otherwise readers will copy-paste a subtly wrong pattern.
Something like:
for (const eventName of this.registeredEvents) {
const eventOptions = this.getEventOptions(eventName)
this.rootContainer.removeEventListener(
eventName,
this.boundHandleDelegatedEvent,
eventOptions
)
}…and the same for portalContainers. Keeps the documentation aligned with the actual behavior and avoids teaching people how to leak capture listeners.
🤖 Prompt for AI Agents
In docs/04-event-system.md around lines 450 to 475, the cleanup() example
removes event listeners without passing the same options used when adding them,
which can leave capture/passive listeners attached; update both the
rootContainer and portalContainers loops to call getEventOptions(eventName) and
pass that options object as the third argument to removeEventListener for each
eventName so removal matches registration and avoids leaking capture listeners.
| "keywords": ["react", "jsx", "mini-react", "virtual-dom"], | ||
| "author": "Your Name", | ||
| "license": "MIT" | ||
| "@types/bun": "latest", |
There was a problem hiding this comment.
Lock down @types/bun to a specific version, dude.
Using "latest" for @types/bun is a footgun waiting to happen. This breaks reproducibility — different CI runs, different machines, different team members can all pull different type definitions. You're basically playing roulette with your build. Pin it to an exact version or at least a minor-range caret.
Example:
- "@types/bun": "latest",
+ "@types/bun": "^1.1.0",📝 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.
| "@types/bun": "latest", | |
| "@types/bun": "^1.1.0", |
🤖 Prompt for AI Agents
In example/package.json around line 23, the dependency "@types/bun" is set to
"latest" which breaks reproducibility; replace "latest" with a pinned version
(preferably an exact version like 0.x.y, or at minimum a caret/minor-range such
as ^0.x.y), update the lockfile (npm/yarn/pnpm) by reinstalling so the concrete
version is recorded, and commit the updated package.json and lockfile to ensure
CI and teammates install the same types.
| #!/bin/bash | ||
| # Setup git hooks for the repository | ||
|
|
||
| HOOKS_DIR=".git/hooks" | ||
| HOOK_FILE="$HOOKS_DIR/pre-commit" | ||
|
|
||
| echo "📦 Setting up git hooks..." | ||
|
|
||
| # Create pre-commit hook |
There was a problem hiding this comment.
Main script needs better error handling and validation.
The setup script itself should also validate that the .git directory exists and handle edge cases:
#!/bin/bash
# Setup git hooks for the repository
+if [ ! -d ".git" ]; then
+ echo "❌ Not in a git repository root"
+ exit 1
+fi
+
HOOKS_DIR=".git/hooks"
HOOK_FILE="$HOOKS_DIR/pre-commit"
+
+if [ ! -d "$HOOKS_DIR" ]; then
+ mkdir -p "$HOOKS_DIR"
+fi
echo "📦 Setting up git hooks..."This is just defensive coding—you're setting up infrastructure, so make it bulletproof.
🤖 Prompt for AI Agents
In scripts/setup-hooks.sh around lines 1 to 9, add defensive validation and
error handling: verify the repository root by checking for a .git directory and
exit with a clear message if missing; ensure HOOKS_DIR exists (create it and
handle mkdir failures), validate you can write to HOOK_FILE (avoid clobbering an
existing hook unless backed up or explicit overwrite), check write operations
succeed and set the executable bit on the hook file, and propagate non-zero exit
codes on failures so callers can detect errors.
| cat > "$HOOK_FILE" << 'EOF' | ||
| #!/bin/sh | ||
| # Pre-commit hook that formats code with Biome | ||
|
|
||
| # Get list of staged files (only .ts, .tsx, .js, .jsx, .json files) | ||
| STAGED_FILES=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\.(ts|tsx|js|jsx|json)$') | ||
|
|
||
| if [ -z "$STAGED_FILES" ]; then | ||
| echo "No staged files to format" | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "🎨 Formatting staged files with Biome..." | ||
|
|
||
| # Format the staged files | ||
| echo "$STAGED_FILES" | xargs bun biome format --write | ||
|
|
||
| # Add the formatted files back to staging | ||
| echo "$STAGED_FILES" | xargs git add | ||
|
|
||
| echo "✅ Formatting complete" | ||
| exit 0 | ||
| EOF |
There was a problem hiding this comment.
Yo, this hook has some reliability issues that'll bite you in production.
Okay so let me be real with you—this hook is going to silently fail in ways that are gonna be chef's kiss annoying to debug. Let me break down the architectural problems:
Critical: Hook always exits 0 (line 31)
-echo "$STAGED_FILES" | xargs bun biome format --write
-
-# Add the formatted files back to staging
-echo "$STAGED_FILES" | xargs git add
-
-echo "✅ Formatting complete"
-exit 0
+echo "$STAGED_FILES" | xargs bun biome format --write
+if [ $? -ne 0 ]; then
+ echo "❌ Biome formatting failed - commit aborted"
+ exit 1
+fi
+
+# Add the formatted files back to staging
+echo "$STAGED_FILES" | xargs git add
+
+echo "✅ Formatting complete"
+exit 0If biome fails (missing dep, permission issue, whatever), the hook still exits 0 and your unformatted code gets committed. That's bad architecture, man.
Major: xargs will explode on spaces in filenames (lines 25, 28)
-echo "$STAGED_FILES" | xargs bun biome format --write
+echo "$STAGED_FILES" | xargs -0 bun biome format --writeOr better yet, use proper null-terminated output from the git command to avoid any shell expansion weirdness.
Major: sh vs bash inconsistency (line 11)
The main script is bash (line 1) but the hook is sh. You're mixing runtimes. Pick one—bash is more portable for this kind of scripting anyway.
Major: No validation that bun/biome exist (line 25)
What happens when someone clones this on a fresh system and tries to commit before running setup? They get a cryptic "bun: command not found" deep in their git hook. Defensive check is cheap:
+if ! command -v bun &> /dev/null; then
+ echo "❌ bun not found - install bun first"
+ exit 1
+fiMinor: Silent overwrites existing hooks (line 10)
If someone already has a pre-commit hook, this just overwrites it without warning. Could add:
+if [ -f "$HOOK_FILE" ]; then
+ echo "⚠️ Pre-commit hook already exists, backing up to ${HOOK_FILE}.bak"
+ cp "$HOOK_FILE" "$HOOK_FILE.bak"
+fi📝 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.
| cat > "$HOOK_FILE" << 'EOF' | |
| #!/bin/sh | |
| # Pre-commit hook that formats code with Biome | |
| # Get list of staged files (only .ts, .tsx, .js, .jsx, .json files) | |
| STAGED_FILES=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\.(ts|tsx|js|jsx|json)$') | |
| if [ -z "$STAGED_FILES" ]; then | |
| echo "No staged files to format" | |
| exit 0 | |
| fi | |
| echo "🎨 Formatting staged files with Biome..." | |
| # Format the staged files | |
| echo "$STAGED_FILES" | xargs bun biome format --write | |
| # Add the formatted files back to staging | |
| echo "$STAGED_FILES" | xargs git add | |
| echo "✅ Formatting complete" | |
| exit 0 | |
| EOF | |
| cat > "$HOOK_FILE" << 'EOF' | |
| #!/bin/sh | |
| # Pre-commit hook that formats code with Biome | |
| # Get list of staged files (only .ts, .tsx, .js, .jsx, .json files) | |
| STAGED_FILES=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\.(ts|tsx|js|jsx|json)$') | |
| if [ -z "$STAGED_FILES" ]; then | |
| echo "No staged files to format" | |
| exit 0 | |
| fi | |
| echo "🎨 Formatting staged files with Biome..." | |
| # Format the staged files | |
| echo "$STAGED_FILES" | xargs bun biome format --write | |
| if [ $? -ne 0 ]; then | |
| echo "❌ Biome formatting failed - commit aborted" | |
| exit 1 | |
| fi | |
| # Add the formatted files back to staging | |
| echo "$STAGED_FILES" | xargs git add | |
| echo "✅ Formatting complete" | |
| exit 0 | |
| EOF |
| # Get list of staged files (only .ts, .tsx, .js, .jsx, .json files) | ||
| STAGED_FILES=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\.(ts|tsx|js|jsx|json)$') |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Git command piping + grep is fragile for file filtering.
Better approach uses git's built-in filtering and null terminators:
-# Get list of staged files (only .ts, .tsx, .js, .jsx, .json files)
-STAGED_FILES=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\.(ts|tsx|js|jsx|json)$')
+# Get list of staged files (only .ts, .tsx, .js, .jsx, .json files)
+# Use null terminators to handle filenames with spaces/special chars safely
+STAGED_FILES=$(git diff --cached --name-only -z --diff-filter=ACM | tr '\0' '\n')This handles edge cases better and is more portable across shells.
📝 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.
| # Get list of staged files (only .ts, .tsx, .js, .jsx, .json files) | |
| STAGED_FILES=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\.(ts|tsx|js|jsx|json)$') | |
| # Get list of staged files (only .ts, .tsx, .js, .jsx, .json files) | |
| # Use null terminators to handle filenames with spaces/special chars safely | |
| STAGED_FILES=$(git diff --cached --name-only -z --diff-filter=ACM | grep -zE '\.(ts|tsx|js|jsx|json)$' | tr '\0' '\n') |
🤖 Prompt for AI Agents
In scripts/setup-hooks.sh around lines 14 to 15, the current pipeline uses grep
to filter staged files which is fragile; replace it with git's null-terminated
output and safe shell reading: use git diff --cached --name-only
--diff-filter=ACM -z and then read the entries with a null delimiter (e.g., read
-r -d '' in a loop or use mapfile -d '') to build the list/array of staged files
limited to the desired extensions by filtering in-shell (or with a simple
parameter expansion/conditional) rather than relying on grep; ensure the
resulting variable handles filenames with spaces, newlines, and special
characters correctly.
| /** | ||
| * Comprehensive tests for effect tag combinations | ||
| * | ||
| * Tests cover: | ||
| * - Bitwise flag operations | ||
| * - Combined effect tags (Placement + Update) | ||
| * - Effect tag preservation during reconciliation | ||
| * - Effect tag propagation through tree | ||
| */ | ||
|
|
||
| import { describe, expect, test } from "bun:test"; | ||
| import { completeWork } from "../../src/fiber/completeWork"; | ||
| import { createFiber } from "../../src/fiber/fiberCreation"; | ||
| import { | ||
| Deletion, | ||
| NoEffect, | ||
| Placement, | ||
| UpdateEffect, | ||
| hasEffectTag, | ||
| } from "../../src/fiber/fiberFlags"; | ||
| import { reconcileChildren } from "../../src/fiber/reconcileChildren"; | ||
| import type { Fiber } from "../../src/fiber/types"; | ||
|
|
||
| describe("Effect Tags - Bitwise Flags", () => { | ||
| describe("Basic Flag Operations", () => { | ||
| test("NoEffect should be 0", () => { | ||
| expect(NoEffect).toBe(0); | ||
| }); | ||
|
|
||
| test("flags should be unique powers of 2", () => { | ||
| expect(Placement).toBe(0b0001); // 1 | ||
| expect(UpdateEffect).toBe(0b0010); // 2 | ||
| expect(Deletion).toBe(0b0100); // 4 | ||
| }); | ||
|
|
||
| test("hasEffectTag should correctly identify single flags", () => { | ||
| expect(hasEffectTag(Placement, Placement)).toBe(true); | ||
| expect(hasEffectTag(UpdateEffect, UpdateEffect)).toBe(true); | ||
| expect(hasEffectTag(Deletion, Deletion)).toBe(true); | ||
|
|
||
| expect(hasEffectTag(Placement, UpdateEffect)).toBe(false); | ||
| expect(hasEffectTag(UpdateEffect, Deletion)).toBe(false); | ||
| expect(hasEffectTag(NoEffect, Placement)).toBe(false); | ||
| }); | ||
|
|
||
| test("bitwise OR should combine flags", () => { | ||
| const combined = Placement | UpdateEffect; | ||
| expect(combined).toBe(0b0011); // 3 | ||
|
|
||
| expect(hasEffectTag(combined, Placement)).toBe(true); | ||
| expect(hasEffectTag(combined, UpdateEffect)).toBe(true); | ||
| expect(hasEffectTag(combined, Deletion)).toBe(false); | ||
| }); | ||
|
|
||
| test("should support all three flags combined", () => { | ||
| const allFlags = Placement | UpdateEffect | Deletion; | ||
| expect(allFlags).toBe(0b0111); // 7 | ||
|
|
||
| expect(hasEffectTag(allFlags, Placement)).toBe(true); | ||
| expect(hasEffectTag(allFlags, UpdateEffect)).toBe(true); | ||
| expect(hasEffectTag(allFlags, Deletion)).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Effect Tag Preservation in completeWork", () => { | ||
| test("should preserve Placement flag when adding Update flag", () => { | ||
| const current = createFiber("div", { id: "old" }, null); | ||
| current.stateNode = document.createElement("div"); | ||
| current.memoizedProps = { id: "old" }; | ||
|
|
||
| const wip = createFiber("div", { id: "new" }, null); | ||
| wip.stateNode = current.stateNode; | ||
| wip.effectTag = Placement; // Already marked for placement | ||
|
|
||
| completeWork(current, wip); | ||
|
|
||
| // Should have both Placement and Update | ||
| expect(hasEffectTag(wip.effectTag, Placement)).toBe(true); | ||
| expect(hasEffectTag(wip.effectTag, UpdateEffect)).toBe(true); | ||
| }); | ||
|
|
||
| test("should not add Update flag if props unchanged", () => { | ||
| const current = createFiber("div", { id: "same" }, null); | ||
| current.stateNode = document.createElement("div"); | ||
| current.memoizedProps = { id: "same", children: [] }; | ||
|
|
||
| const wip = createFiber("div", { id: "same", children: [] }, null); | ||
| wip.stateNode = current.stateNode; | ||
| wip.effectTag = Placement; | ||
|
|
||
| completeWork(current, wip); | ||
|
|
||
| // Should still have Placement, but not Update | ||
| expect(hasEffectTag(wip.effectTag, Placement)).toBe(true); | ||
| expect(hasEffectTag(wip.effectTag, UpdateEffect)).toBe(false); | ||
| }); | ||
|
|
||
| test("should add Update flag for text content changes while preserving Placement", () => { | ||
| const current = createFiber("TEXT_ELEMENT", { nodeValue: "old" }, null); | ||
| current.stateNode = document.createTextNode("old"); | ||
| current.memoizedProps = { nodeValue: "old" }; | ||
|
|
||
| const wip = createFiber("TEXT_ELEMENT", { nodeValue: "new" }, null); | ||
| wip.stateNode = current.stateNode; | ||
| wip.effectTag = Placement; | ||
|
|
||
| completeWork(current, wip); | ||
|
|
||
| expect(hasEffectTag(wip.effectTag, Placement)).toBe(true); | ||
| expect(hasEffectTag(wip.effectTag, UpdateEffect)).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Effect Tag Combinations in Reconciliation", () => { | ||
| test("should mark moved nodes with proper flags", () => { | ||
| const parent = createFiber("div", {}, null); | ||
|
|
||
| // Create existing children | ||
| const child1 = createFiber("div", {}, "a"); | ||
| const child2 = createFiber("div", {}, "b"); | ||
| child1.sibling = child2; | ||
| parent.child = child1; | ||
|
|
||
| // Reconcile with swapped order | ||
| const newChildren = [ | ||
| { type: "div", props: { key: "b", children: [] } }, | ||
| { type: "div", props: { key: "a", children: [] } }, | ||
| ]; | ||
|
|
||
| reconcileChildren(child1, parent, newChildren); | ||
|
|
||
| // Child that moved should be marked for placement | ||
| const firstChild = parent.child; | ||
| expect(firstChild?.key).toBe("b"); | ||
| expect(hasEffectTag(firstChild?.effectTag || 0, Placement)).toBe(true); | ||
| }); | ||
|
|
||
| test("should handle adding new children among existing ones", () => { | ||
| const parent = createFiber("div", {}, null); | ||
|
|
||
| const child1 = createFiber("div", {}, "a"); | ||
| parent.child = child1; | ||
|
|
||
| // Add new children before and after | ||
| const newChildren = [ | ||
| { type: "div", props: { key: "new1", children: [] } }, | ||
| { type: "div", props: { key: "a", children: [] } }, | ||
| { type: "div", props: { key: "new2", children: [] } }, | ||
| ]; | ||
|
|
||
| reconcileChildren(child1, parent, newChildren); | ||
|
|
||
| // Walk and verify flags | ||
| let current: Fiber | null = parent.child; | ||
| const results = []; | ||
| while (current) { | ||
| results.push({ | ||
| key: current.key, | ||
| hasPlacement: hasEffectTag(current.effectTag, Placement), | ||
| hasUpdate: hasEffectTag(current.effectTag, UpdateEffect), | ||
| }); | ||
| current = current.sibling; | ||
| } | ||
|
|
||
| // new1 is new -> Placement | ||
| // "a" is reused -> gets Placement because it moved position | ||
| // new2 is new -> Placement | ||
| expect(results).toEqual([ | ||
| { key: "new1", hasPlacement: true, hasUpdate: false }, | ||
| { key: "a", hasPlacement: true, hasUpdate: false }, | ||
| { key: "new2", hasPlacement: true, hasUpdate: false }, | ||
| ]); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Effect Tag Edge Cases", () => { | ||
| test("should handle clearing all flags", () => { | ||
| let effectTag = Placement | UpdateEffect | Deletion; | ||
| effectTag = NoEffect; | ||
|
|
||
| expect(effectTag).toBe(0); | ||
| expect(hasEffectTag(effectTag, Placement)).toBe(false); | ||
| expect(hasEffectTag(effectTag, UpdateEffect)).toBe(false); | ||
| expect(hasEffectTag(effectTag, Deletion)).toBe(false); | ||
| }); | ||
|
|
||
| test("should handle removing specific flag with bitwise AND NOT", () => { | ||
| let effectTag = Placement | UpdateEffect; | ||
|
|
||
| // Remove Placement flag | ||
| effectTag = effectTag & ~Placement; | ||
|
|
||
| expect(hasEffectTag(effectTag, Placement)).toBe(false); | ||
| expect(hasEffectTag(effectTag, UpdateEffect)).toBe(true); | ||
| }); | ||
|
|
||
| test("should handle toggling flags", () => { | ||
| let effectTag = Placement; | ||
|
|
||
| // Toggle UpdateEffect on | ||
| effectTag = effectTag | UpdateEffect; | ||
| expect(hasEffectTag(effectTag, UpdateEffect)).toBe(true); | ||
|
|
||
| // Toggle UpdateEffect off | ||
| effectTag = effectTag & ~UpdateEffect; | ||
| expect(hasEffectTag(effectTag, UpdateEffect)).toBe(false); | ||
| expect(hasEffectTag(effectTag, Placement)).toBe(true); | ||
| }); | ||
|
|
||
| test("should handle checking multiple flags at once", () => { | ||
| const effectTag = Placement | UpdateEffect; | ||
|
|
||
| // Check if has any of the flags | ||
| const hasAnyMutation = | ||
| hasEffectTag(effectTag, Placement) || | ||
| hasEffectTag(effectTag, UpdateEffect) || | ||
| hasEffectTag(effectTag, Deletion); | ||
|
|
||
| expect(hasAnyMutation).toBe(true); | ||
|
|
||
| // Check if has all flags | ||
| const hasBothPlacementAndUpdate = | ||
| hasEffectTag(effectTag, Placement) && | ||
| hasEffectTag(effectTag, UpdateEffect); | ||
|
|
||
| expect(hasBothPlacementAndUpdate).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Real-World Scenarios", () => { | ||
| test("moving element with changed props should have both Placement and Update", () => { | ||
| const parent = createFiber("div", {}, null); | ||
|
|
||
| // Existing: [A, B, C] | ||
| const childA = createFiber("div", { className: "old" }, "a"); | ||
| const childB = createFiber("div", {}, "b"); | ||
| const childC = createFiber("div", {}, "c"); | ||
|
|
||
| childA.sibling = childB; | ||
| childB.sibling = childC; | ||
| parent.child = childA; | ||
|
|
||
| // New: [C, B, A(with new props)] | ||
| const newChildren = [ | ||
| { type: "div", props: { key: "c", children: [] } }, | ||
| { type: "div", props: { key: "b", children: [] } }, | ||
| { type: "div", props: { key: "a", className: "new", children: [] } }, | ||
| ]; | ||
|
|
||
| reconcileChildren(childA, parent, newChildren); | ||
|
|
||
| // Find the reconciled child A | ||
| let current: Fiber | null = parent.child; | ||
| let reconciledA = null; | ||
| while (current) { | ||
| if (current.key === "a") { | ||
| reconciledA = current; | ||
| break; | ||
| } | ||
| current = current.sibling; | ||
| } | ||
|
|
||
| // Child A moved to different position | ||
| expect(hasEffectTag(reconciledA?.effectTag || 0, Placement)).toBe(true); | ||
| }); | ||
|
|
||
| test("text node moving and changing content should have both flags", () => { | ||
| const parent = createFiber("div", {}, null); | ||
|
|
||
| const text = createFiber("TEXT_ELEMENT", { nodeValue: "old" }, null); | ||
| text.stateNode = document.createTextNode("old"); | ||
| text.memoizedProps = { nodeValue: "old" }; | ||
| parent.child = text; | ||
|
|
||
| // Complete work on text with Placement flag and changed content | ||
| const wipText = createFiber("TEXT_ELEMENT", { nodeValue: "new" }, null); | ||
| wipText.stateNode = text.stateNode; | ||
| wipText.effectTag = Placement; | ||
|
|
||
| completeWork(text, wipText); | ||
|
|
||
| expect(hasEffectTag(wipText.effectTag, Placement)).toBe(true); | ||
| expect(hasEffectTag(wipText.effectTag, UpdateEffect)).toBe(true); | ||
| }); | ||
|
|
||
| test("element being deleted should only have Deletion flag", () => { | ||
| const parent = createFiber("div", {}, null); | ||
| const child = createFiber("div", {}, "a"); | ||
| child.effectTag = Deletion; | ||
|
|
||
| parent.child = child; | ||
|
|
||
| // Should not have other flags | ||
| expect(hasEffectTag(child.effectTag, Deletion)).toBe(true); | ||
| expect(hasEffectTag(child.effectTag, Placement)).toBe(false); | ||
| expect(hasEffectTag(child.effectTag, UpdateEffect)).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Effect Tag Propagation", () => { | ||
| test("should accumulate effect tags on parent during complete work", () => { | ||
| const parent = createFiber("div", {}, null); | ||
|
|
||
| // Create children with different effects | ||
| const child1 = createFiber("div", {}, null); | ||
| child1.effectTag = Placement; | ||
|
|
||
| const child2 = createFiber("div", {}, null); | ||
| child2.effectTag = UpdateEffect; | ||
|
|
||
| child1.sibling = child2; | ||
| parent.child = child1; | ||
|
|
||
| // Each child should maintain its own effect tag | ||
| expect(child1.effectTag).toBe(Placement); | ||
| expect(child2.effectTag).toBe(UpdateEffect); | ||
| }); | ||
|
|
||
| test("should not inherit parent effect tags", () => { | ||
| const parent = createFiber("div", {}, null); | ||
| parent.effectTag = UpdateEffect; | ||
|
|
||
| const child = createFiber("div", {}, null); | ||
| child.effectTag = NoEffect; | ||
| parent.child = child; | ||
|
|
||
| // Child should have its own effect tag, not parent's | ||
| expect(child.effectTag).toBe(NoEffect); | ||
| expect(hasEffectTag(child.effectTag, UpdateEffect)).toBe(false); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Overall: This test file is absolutely STACKED
Okay, let's be real - this is some seriously comprehensive test coverage. You're testing:
- ✅ Basic bitwise operations
- ✅ Flag preservation during completeWork
- ✅ Reconciliation scenarios
- ✅ Edge cases with bitwise manipulation
- ✅ Real-world scenarios
- ✅ Propagation behavior
The structure is clean, the test names are descriptive, and you're covering both the happy path AND the sad path. The binary literals (0b0001) for testing bit patterns is exactly the right move.
Main issues are:
- That
|| 0fallback pattern needs to die (lines 135, 264) - we're writing TypeScript, act like it - Consider extracting the sibling-walking pattern into a test helper for consistency
- One test name doesn't match what it's actually testing (line 301)
But these are nitpicks on what is otherwise a really solid test suite. You're clearly thinking about the edge cases and potential failure modes, which is what you NEED in a fiber reconciliation system.
Ship it, but fix those TypeScript patterns first.
| reconcileChildren(child1, parent, newChildren); | ||
|
|
||
| // Child that moved should be marked for placement | ||
| const firstChild = parent.child; | ||
| expect(firstChild?.key).toBe("b"); | ||
| expect(hasEffectTag(firstChild?.effectTag || 0, Placement)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Bro, what is this || 0 business?
expect(hasEffectTag(firstChild?.effectTag || 0, Placement)).toBe(true);Nah nah nah, we're writing TypeScript here, not JavaScript with extra steps! This optional chaining + || 0 pattern is exactly the kind of thing that makes TypeScript folks lose their minds. If firstChild is null/undefined, your test should FAIL, not silently use 0.
Do it like this:
- // Child that moved should be marked for placement
- const firstChild = parent.child;
- expect(firstChild?.key).toBe("b");
- expect(hasEffectTag(firstChild?.effectTag || 0, Placement)).toBe(true);
+ // Child that moved should be marked for placement
+ const firstChild = parent.child;
+ expect(firstChild).toBeDefined();
+ expect(firstChild?.key).toBe("b");
+ expect(hasEffectTag(firstChild!.effectTag, Placement)).toBe(true);Now TypeScript knows firstChild exists, and if it doesn't, the test fails with a CLEAR message instead of some weird 0 fallback.
🤖 Prompt for AI Agents
In tests/fiber/effectTags.test.ts around lines 130-136, remove the
optional-chaining-with-fallback pattern and make the test fail if firstChild is
missing: first assert firstChild is defined (e.g.,
expect(firstChild).toBeDefined() or not.toBeNull()), then call hasEffectTag with
the actual effectTag (use non-null assertion firstChild!.effectTag) instead of
falling back to 0; this ensures the test fails loudly if firstChild is absent
and removes the "|| 0" hack.
| let current: Fiber | null = parent.child; | ||
| const results = []; | ||
| while (current) { | ||
| results.push({ | ||
| key: current.key, | ||
| hasPlacement: hasEffectTag(current.effectTag, Placement), | ||
| hasUpdate: hasEffectTag(current.effectTag, UpdateEffect), | ||
| }); | ||
| current = current.sibling; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mutable variables in the loop - it's giving JavaScript
Look, I get it, walking a linked list with a while loop is classic. But we're doing the mutable reassignment dance here:
let current: Fiber | null = parent.child;
const results = [];
while (current) {
results.push({...});
current = current.sibling;
}In tests, readability matters more than being a functional programming purist, so this is fine. But if you're doing this pattern in 50 places across your test suite, maybe extract a helper:
function walkSiblings(fiber: Fiber | null, fn: (f: Fiber) => void): void {
let current = fiber;
while (current) {
fn(current);
current = current.sibling;
}
}Then your test becomes:
const results: Array<{key: string, hasPlacement: boolean, hasUpdate: boolean}> = [];
walkSiblings(parent.child, (f) => {
results.push({
key: f.key,
hasPlacement: hasEffectTag(f.effectTag, Placement),
hasUpdate: hasEffectTag(f.effectTag, UpdateEffect),
});
});Not required here, just something to think about for your test utilities.
🤖 Prompt for AI Agents
In tests/fiber/effectTags.test.ts around lines 154 to 163, the loop mutably
reassigns a local variable while walking sibling fibers which is repeated
pattern; extract a reusable helper like walkSiblings that accepts a starting
Fiber|null and a callback, use it in this test to collect results by invoking
the callback for each sibling instead of mutating current in-place, and update
the test to call that helper to build the results array for better readability
and reuse.
| reconcileChildren(childA, parent, newChildren); | ||
|
|
||
| // Find the reconciled child A | ||
| let current: Fiber | null = parent.child; | ||
| let reconciledA = null; | ||
| while (current) { | ||
| if (current.key === "a") { | ||
| reconciledA = current; | ||
| break; | ||
| } | ||
| current = current.sibling; | ||
| } | ||
|
|
||
| // Child A moved to different position | ||
| expect(hasEffectTag(reconciledA?.effectTag || 0, Placement)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Here we go again with the || 0 pattern
expect(hasEffectTag(reconciledA?.effectTag || 0, Placement)).toBe(true);Bro! We JUST talked about this! Same issue as line 135. You're walking the tree to find child A (lines 253-261), and then you're like "well, maybe it doesn't exist, so I'll just use 0 as a fallback."
If reconciledA is null here, your test should FAIL LOUDLY because that means reconciliation didn't work. The || 0 is hiding potential bugs in your reconciliation logic.
Fix it:
// Find the reconciled child A
let current: Fiber | null = parent.child;
let reconciledA = null;
while (current) {
if (current.key === "a") {
reconciledA = current;
break;
}
current = current.sibling;
}
+ expect(reconciledA).toBeDefined();
// Child A moved to different position
- expect(hasEffectTag(reconciledA?.effectTag || 0, Placement)).toBe(true);
+ expect(hasEffectTag(reconciledA!.effectTag, Placement)).toBe(true);While we're here, same comment about extracting the sibling-walking pattern into a helper applies.
🤖 Prompt for AI Agents
In tests/fiber/effectTags.test.ts around lines 250 to 265, the test masks a
missing reconciled node by using a fallback `|| 0`; instead, assert the node
exists and then check its effectTag. Replace the `|| 0` usage by first asserting
`reconciledA` is not null (so the test fails loudly if reconciliation failed)
and then use the non-null value to call hasEffectTag on reconciledA.effectTag;
also extract the sibling-walking pattern into a small helper function (e.g.,
findChildByKey(parent, key)) and use it here to keep the test concise and
reusable.
| test("should accumulate effect tags on parent during complete work", () => { | ||
| const parent = createFiber("div", {}, null); | ||
|
|
||
| // Create children with different effects | ||
| const child1 = createFiber("div", {}, null); | ||
| child1.effectTag = Placement; | ||
|
|
||
| const child2 = createFiber("div", {}, null); | ||
| child2.effectTag = UpdateEffect; | ||
|
|
||
| child1.sibling = child2; | ||
| parent.child = child1; | ||
|
|
||
| // Each child should maintain its own effect tag | ||
| expect(child1.effectTag).toBe(Placement); | ||
| expect(child2.effectTag).toBe(UpdateEffect); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test name doesn't match what it's testing
The test is called "should accumulate effect tags on parent during complete work" but you're only asserting that children maintain their own tags:
expect(child1.effectTag).toBe(Placement);
expect(child2.effectTag).toBe(UpdateEffect);You're not actually checking if the parent accumulated anything. If this is intentional (maybe parent accumulation happens in a different phase or is tested elsewhere), cool, but the test name is misleading.
Either:
- Rename the test to "should maintain individual effect tags on children"
- Or add assertions about parent accumulation if that's what you meant to test
Also, based on the PR objectives mentioning "effect lists for O(changes) commits", I'm assuming effect accumulation happens via effect list building, not by modifying parent.effectTag directly. If that's the case, definitely rename this test.
🤖 Prompt for AI Agents
In tests/fiber/effectTags.test.ts around lines 301-317 the test name claims
"should accumulate effect tags on parent during complete work" but it only
asserts children keep their own tags; either rename the test to "should maintain
individual effect tags on children" or extend the test to actually trigger the
parent accumulation step (invoke the complete/completeWork logic or effect-list
building used in the repo) and then assert the parent has the expected
accumulated effect (e.g., parent.effectTag includes Placement|UpdateEffect or
parent’s effect list contains the child effects) depending on how accumulation
is implemented.
Critical fixes addressing CodeRabbit feedback: **Fix setter identity thrashing (React contract violation)** - useState and useReducer now return stable function references - Capture fiber at hook initialization instead of creating new closures - Prevents infinite loops from dependency array invalidation - Fixes effects/memos that depend on setState/dispatch **Refactor effect queue into legacy scheduler** - Move effectQueue to dedicated src/hooks/legacyScheduler.ts module - Cleaner separation of concerns - Add resetEffectQueueForTests() to prevent test pollution - Maintain backward compatibility with existing code This fixes major performance issues where every render created new setState/dispatch functions, causing unnecessary re-renders and breaking React's stability guarantees. All 520 tests passing, zero type errors.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/hooks/index.ts (2)
52-65: Hook slot boilerplate is copy‑pasted all over – this is begging to driftThis exact dance:
getCurrentRenderingFiber()- throw if missing
- ensure
fiber.hooks- read
fiber.hookCursor- bump it
- stash
hooks+currentHookIndexis repeated in
useState,useReducer,useEffect,useRef,useMemo,useCallback.This is exactly the kind of copy‑paste that will fall out of sync the moment you add another hook or tweak the rules for one of them. Just rip it into a tiny internal helper and be done with it, something like:
function prepareHookSlot(hookName: string) { const fiber = getCurrentRenderingFiber(); if (!fiber) { throw new Error(`${hookName} must be called inside a functional component`); } if (!fiber.hooks) { fiber.hooks = []; } const hooks = fiber.hooks; const currentHookIndex = fiber.hookCursor; fiber.hookCursor = currentHookIndex + 1; return { fiber, hooks, currentHookIndex }; }Then your hooks read way nicer and are harder to screw up:
export function useState<T>(initialState: T | (() => T)): UseStateHook<T> { const { fiber, hooks, currentHookIndex } = prepareHookSlot("useState"); // ... } export function useEffect(callback: EffectCallback, dependencies?: DependencyList) { const { hooks, currentHookIndex } = prepareHookSlot("useEffect"); // ... }Same behavior, way less surface area for subtle divergence.
Also applies to: 129-142, 206-220, 255-268, 294-307, 349-362
413-418: ExposinggetCurrentRenderingFiberpublicly is still a loaded gunYou’re exporting:
export { setCurrentRenderingFiber, getCurrentRenderingFiber, setCurrentRenderingFiber as setHookContext, } from "../fiber/fiberHooks";This effectively blesses
getCurrentRenderingFiberas public API. Once user code starts poking at fibers directly, you are locked in to the current internal shape and lifecycle forever. That’s how you end up with “we can’t change this field because some random library reads it”.I’d strongly prefer to:
-export { - setCurrentRenderingFiber, - getCurrentRenderingFiber, - setCurrentRenderingFiber as setHookContext, -} from "../fiber/fiberHooks"; +export { + // For internal wiring (beginWork, etc.), import directly from "../fiber/fiberHooks". + // Only expose the alias here for external callers. + setCurrentRenderingFiber as setHookContext, +} from "../fiber/fiberHooks";Internal code can and should import the full fiber API from
../fiber/fiberHooksdirectly; the public hooks surface should only expose the high‑level hook context alias. Don’t hand your users the fiber shotgun.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/hooks/index.ts(8 hunks)src/hooks/legacyScheduler.ts(1 hunks)
🧰 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/hooks/legacyScheduler.ts (1)
src/hooks/index.ts (3)
scheduleEffect(40-40)getEffectQueue(41-41)resetEffectQueueForTests(42-42)
🔇 Additional comments (4)
src/hooks/index.ts (4)
75-105: Captured fiber/hooks in dispatch are fine today, but double‑buffering will stress thisThe
useState/useReducerimplementations close over:
capturedFiber = fiberconst hooks = fiber.hooksconst currentHookIndex = fiber.hookCursor(for reducer dispatch)and then pass those into
dispatchSetState/dispatchReducerAction.That’s totally fine as long as:
- You mutate a single fiber instance across renders (no real double‑buffering), or
- Your dispatch functions know how to walk from a “stale” captured fiber to the currently mounted one (e.g., via
alternate/root walking), andhooksalways points at the authoritative hook array for that fiber slot.If you ever go full React‑style current/workInProgress swapping, this pattern can turn into “dispatch targets the dead alternate” bugs real fast.
I’d at least sanity‑check that your
dispatchSetState/dispatchReducerActionimplementations treat the captured fiber as an identity token for the component, not as “the object that currently owns the hooks array”. If that’s not true, this is going to be the source of some truly cursed heisenbugs later.Also applies to: 151-193
202-246: Effect hook wiring looks solid; just make sure commit clearsneedsRun/setshasRun
useEffectis doing the right bookkeeping here:
- Stores
callback+dependencieson the hook.- Uses
areDepsEqualto decide whether to markneedsRun.- Tracks a
hasRunbit so the first render always schedules the effect.The only thing to be paranoid about is the other half of the contract: the commit phase has to:
- Run
callbackfor hooks withneedsRun === true,- Flip
hasRuntotrue,- Clear
needsRun(and handle cleanup if you support it).If that’s already happening in the fiber commit path, then this implementation is clean and minimal. If not, you’ll end up with effects that fire every commit forever.
345-389: useCallback / useMemo dependency handling is nice and tightThe combo of
areDepsEqual+ “only recompute/update when deps change” for bothuseMemoanduseCallbackis exactly what you want here. The separation between:
hasComputedforuseMemo, and- “just compare deps” for
useCallbackkeeps the semantics straightforward and React‑ish.
Assuming
areDepsEqualdoes the usual “undefined/null means always different” thing, this is totally solid.
391-411: Type re‑exports from./typesare clean and exactly what you wantThe big type re‑export block gives consumers a single place to import all hook‑related types without leaking the internal layout of the hooks module.
No complaints here; this is nice API surface hygiene.
| // Re-export legacy scheduler functions for backward compatibility | ||
| export { | ||
| scheduleEffect, | ||
| getEffectQueue, | ||
| resetEffectQueueForTests, | ||
| } from "./legacyScheduler"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Re‑exporting the legacy scheduler from hooks keeps compat, but maybe don’t bless it
Re‑exporting scheduleEffect / getEffectQueue / resetEffectQueueForTests from the main hooks entrypoint makes this look like part of the blessed public surface.
Given this is explicitly a legacy/testing shim, I’d strongly consider:
- Keeping the re‑export but slapping a loud JSDoc
@deprecatedon the export block here as well, or - Moving these under a clearly “internal/test” namespace (e.g.
mini-react/hooks/testing) so app code doesn’t casually depend on them.
Right now they’re one import away from getting used in production code, and that’s going to make ripping them out later way more painful.
🤖 Prompt for AI Agents
In src/hooks/index.ts around lines 38 to 43, the legacy scheduler re-exports
make those APIs appear public; mark them as deprecated or move them to an
internal/testing namespace. Either add a clear JSDoc block above the export with
@deprecated and a short rationale (e.g., "legacy/testing shim — do not use in
production"), or remove these re-exports from this file and create a new module
such as src/hooks/testing.ts (or src/hooks/internal/testing.ts) that re-exports
scheduleEffect, getEffectQueue, resetEffectQueueForTests; then update any
internal tests/imports to point to the new testing path and remove these names
from the main hooks index export.
| let effectQueue: (() => void)[] = []; | ||
| let isFlushingEffects = false; | ||
|
|
||
| /** | ||
| * Schedule an effect to be run after the current render | ||
| * | ||
| * @deprecated Effects should run in commit phase, not via microtask | ||
| */ | ||
| export function scheduleEffect(effectFn: () => void): void { | ||
| effectQueue.push(effectFn); | ||
|
|
||
| if (!isFlushingEffects) { | ||
| queueMicrotask(flushEffects); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Flush all queued effects | ||
| */ | ||
| function flushEffects(): void { | ||
| if (isFlushingEffects) return; | ||
|
|
||
| isFlushingEffects = true; | ||
|
|
||
| try { | ||
| while (effectQueue.length > 0) { | ||
| const effect = effectQueue.shift(); | ||
| if (effect) { | ||
| effect(); | ||
| } | ||
| } | ||
| } finally { | ||
| isFlushingEffects = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Legacy scheduler semantics are fine, but this microtask pattern is a little spicy
This works, but it's a bit “just trust me bro”:
scheduleEffectwill queue a microtask every time you call it while not currently flushing. In a render that schedules a ton of effects, that’s a pile of redundant microtasks that will just see an empty queue.- Reentrancy during
flushEffectsis fine (you drain until empty), but you’re also relying onisFlushingEffectsboth as a “currently flushing” guard and as a “flush already scheduled” guard, which are subtly different concerns.
I’d split those responsibilities so this is more explicit and future‑proof (especially if you ever change when/how flushEffects gets called):
-let effectQueue: (() => void)[] = [];
-let isFlushingEffects = false;
+let effectQueue: (() => void)[] = [];
+let isFlushingEffects = false;
+let isFlushScheduled = false;
export function scheduleEffect(effectFn: () => void): void {
effectQueue.push(effectFn);
- if (!isFlushingEffects) {
- queueMicrotask(flushEffects);
- }
+ if (!isFlushingEffects && !isFlushScheduled) {
+ isFlushScheduled = true;
+ queueMicrotask(flushEffects);
+ }
}
function flushEffects(): void {
if (isFlushingEffects) return;
isFlushingEffects = true;
+ isFlushScheduled = false;That keeps the behavior the same today but makes it much harder to accidentally regress when the fiber side evolves.
🤖 Prompt for AI Agents
In src/hooks/legacyScheduler.ts around lines 18–52, split the single
isFlushingEffects flag into two booleans (e.g., isFlushing and isFlushScheduled)
so scheduling vs. active flushing are distinct: push the effect onto effectQueue
in scheduleEffect and only queueMicrotask(flushEffects) when a flush is not
already scheduled (set isFlushScheduled = true when scheduling); in flushEffects
clear the scheduled flag, set isFlushing = true while draining the queue
(allowing reentrant scheduleEffect calls to merely append without creating
another microtask), and finally set isFlushing = false; ensure both flags are
reset appropriately in all paths (try/finally).
| /** | ||
| * Get the effect queue for external access | ||
| * | ||
| * @deprecated For testing only | ||
| */ | ||
| export function getEffectQueue(): (() => void)[] { | ||
| return effectQueue; | ||
| } | ||
|
|
||
| /** | ||
| * Reset the effect queue (for testing) | ||
| * | ||
| * This is needed to prevent test pollution when multiple tests | ||
| * schedule effects that outlive the test lifecycle. | ||
| */ | ||
| export function resetEffectQueueForTests(): void { | ||
| effectQueue = []; | ||
| isFlushingEffects = false; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test helpers are good, but exposing the live queue is a foot‑gun
getEffectQueue is marked “for testing only” but returns the mutable internal array. One overeager test can effectQueue.length = 0 or shift() manually and you’re in heisenbug city for every subsequent test.
Given this is purely test‑facing, I’d wrap it so callers can only observe, not mutate:
-export function getEffectQueue(): (() => void)[] {
- return effectQueue;
-}
+export function getEffectQueue(): ReadonlyArray<() => void> {
+ return effectQueue;
+}resetEffectQueueForTests is the one legit mutation hook and already does the right thing. Keep all the sharp bits in one place instead of sprinkling them across tests.
📝 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.
| /** | |
| * Get the effect queue for external access | |
| * | |
| * @deprecated For testing only | |
| */ | |
| export function getEffectQueue(): (() => void)[] { | |
| return effectQueue; | |
| } | |
| /** | |
| * Reset the effect queue (for testing) | |
| * | |
| * This is needed to prevent test pollution when multiple tests | |
| * schedule effects that outlive the test lifecycle. | |
| */ | |
| export function resetEffectQueueForTests(): void { | |
| effectQueue = []; | |
| isFlushingEffects = false; | |
| } | |
| /** | |
| * Get the effect queue for external access | |
| * | |
| * @deprecated For testing only | |
| */ | |
| export function getEffectQueue(): ReadonlyArray<() => void> { | |
| return effectQueue; | |
| } | |
| /** | |
| * Reset the effect queue (for testing) | |
| * | |
| * This is needed to prevent test pollution when multiple tests | |
| * schedule effects that outlive the test lifecycle. | |
| */ | |
| export function resetEffectQueueForTests(): void { | |
| effectQueue = []; | |
| isFlushingEffects = false; | |
| } |
🤖 Prompt for AI Agents
In src/hooks/legacyScheduler.ts around lines 54 to 72, getEffectQueue currently
returns the live mutable effectQueue which lets tests mutate internal state;
change it to return a non‑mutating view (e.g. a shallow copy or a frozen copy)
so callers can observe but not modify the internal array, and keep
resetEffectQueueForTests as the sole mutating API for tests.
Fiber Architecture Implementation
Overview
This PR replaces MiniReact's recursive rendering system with a complete fiber-based architecture. The fiber architecture is how modern React works internally, and it gives us a solid foundation for future features like concurrent mode and Suspense while improving performance and maintainability.
Why Fiber?
The old recursive approach had some fundamental limitations that were becoming problematic:
Stack depth issues - Deep component trees could blow the stack. With recursion, there's no way around this except limiting tree depth.
Non-interruptible rendering - Once rendering started, it had to complete. This blocked the main thread and could make UIs feel sluggish.
No concurrent mode support - You can't pause or prioritize recursive work. Updates were all-or-nothing.
Limited error recovery - Error boundaries are hard to implement properly when you're deep in a recursive call stack.
Performance bottlenecks - Re-rendering entire subtrees even when only small parts changed, because there wasn't a good way to track what actually needed updating.
Fiber solves all of these by breaking rendering into small units of work that can be paused, resumed, and prioritized.
What's a Fiber?
A fiber is just a JavaScript object that represents a component instance and tracks its work. Instead of using the call stack, we build an explicit tree of fiber objects linked by pointers. This lets us traverse the tree iteratively and maintain our own work queue.
The key insight is maintaining two trees - the current tree (what's on screen) and the work-in-progress tree (what we're building). This double-buffering pattern means we can work on updates without disrupting the current UI, then swap the trees atomically when we're done.
Key Features
Core Architecture
Double-buffering - Current and work-in-progress trees linked through the
alternatepointer. Enables safe, interruptible updates.Incremental rendering - Work processed unit-by-unit using iterative tree traversal. No recursion means no stack depth issues.
Effect lists - Instead of walking the entire tree during commit, we maintain a linked list of only the fibers that have changes. This makes commit phase O(changes) instead of O(total nodes).
Work loop - Clean separation between render phase (building the tree) and commit phase (applying changes to DOM).
Reconciliation Engine
O(n) diffing - Key-based reconciliation keeps things fast. We make smart assumptions (same type = reuse, different type = replace) to avoid O(n³) tree comparison.
Mixed keyed/unkeyed handling - Properly handles edge cases when you mix keyed and unkeyed children in the same list.
Type-based reuse - When component types match, we reuse the fiber and just update it. When types differ, we know we need a full replacement.
Optimized paths - Special fast paths for common cases like single children or empty children.
Event System
Full delegation - All events delegated to the root container. Better performance and easier to manage.
Synthetic events - Normalized cross-browser event handling. Your event handlers get consistent objects regardless of browser.
Portal event bubbling - Events bubble through the React component tree, not the DOM tree. This is important for portals to work correctly.
Proper cleanup - Event listeners removed when components unmount. No memory leaks.
Hooks Integration
Fiber-based storage - Hooks stored directly on fiber objects. Each fiber maintains its own hook array.
Update queues - State updates are queued and batched. Multiple setState calls in the same render get processed together.
Effect scheduling - Effects run at the right time with proper cleanup handling.
Architecture
The fiber system is split into focused modules:
Each module has a single clear responsibility. The work loop coordinates everything, begin work processes components, reconciliation diffs children, complete work builds DOM nodes, and commit work applies changes.
Documentation
Added comprehensive docs that explain how everything works:
Fiber Architecture (
docs/01-fiber-architecture.md) - The core concepts, double-buffering pattern, and tree structure.Work Loop (
docs/02-work-loop.md) - How rendering is coordinated, the traversal algorithm, and where work happens.Reconciliation (
docs/03-reconciliation.md) - The diffing algorithm, key-based matching, and how we handle updates efficiently.Event System (
docs/04-event-system.md) - Event delegation, synthetic events, and how portal bubbling works.Hooks System (
docs/05-hooks-system.md) - Hook storage on fibers, update queues, and effect scheduling.Overview (
docs/README.md) - Navigation guide and getting started.Each doc includes practical examples, implementation notes, and thoughts on future enhancements. The goal is to make the codebase approachable for contributors.
Example App Rewrite
The example app got a complete overhaul with modern tooling:
ElysiaJS server - Type-safe server with proper MIME types. Fast and lightweight.
Full TypeScript - Everything properly typed, no
anytypes anywhere in the app code.Comprehensive demos showing all MiniReact features:
useStateuseEffectwith cleanupuseCallbackanduseMemofor performanceuseContextThe app is clean, well-documented, and shows best practices for using MiniReact.
Technical Details
TypeScript
Fixed all type errors properly without resorting to
any:stateNode:Node | FiberRoot | PortalContainer | null@ts-expect-errorcomments only where our types intentionally differ from React@ts-nocheckin test files that deliberately use incomplete props for testinganytypes in source code (tests use it only for edge case testing)Linting
noNonNullAssertionsince tests often assert non-null)Testing
All existing tests pass plus comprehensive new fiber tests:
Integration tests - Full render cycles, updates, unmounting, effects
Unit tests - Individual fiber operations in isolation
Edge cases - Mixed keyed/unkeyed children, portal events, synthetic event properties
Reconciliation tests - Key matching, element moves, insertions, deletions
Hook tests - State updates, effect cleanup, batching behavior
Test coverage maintained at 100% with 484 passing tests.
Performance
The fiber architecture improves performance in several ways:
Efficient updates - Only changed nodes are processed during commit. If 5 nodes changed in a 10,000 node tree, we process 5 nodes.
No recursion - Iterative traversal means no stack depth limits. Render trees as deep as you want.
Fiber pooling - The
alternatepointer lets us reuse fiber objects between renders instead of allocating new ones.Effect lists - Building a linked list of changes during render means commit phase is O(changes) not O(nodes).
Early bailout - When props haven't changed and there's no local state update, we skip entire subtrees.
Rendering is still synchronous for now (predictable behavior, simpler implementation), but the architecture is ready for concurrent mode and time-slicing when we want to add them.
Breaking Changes
None for the public API. Everything that worked before still works:
Internally, the old
VDOMInstancesystem is gone, but that was already deprecated and not part of the public API.Testing
To verify everything works:
All checks pass. 484 tests, zero type errors, zero lint errors.
Future Work
This implementation sets us up for some really interesting features:
Concurrent mode - Time-slice rendering so the UI stays responsive. High-priority updates (typing) can interrupt low-priority ones (data fetching).
Suspense - Async rendering with proper loading states. Components can "suspend" while waiting for data.
Error boundaries - Catch errors in component trees and show fallback UI. The fiber tree structure makes this straightforward.
Profiler - Measure render times and identify performance bottlenecks. Fiber gives us the hooks we need.
Server components - Better server-side rendering with streaming and selective hydration.
The hard part (the architecture) is done. Adding these features is mostly incremental work now.
Migration
No migration needed. This is a drop-in replacement for the old rendering system. Existing code works without any changes.
References
This implementation draws heavily from React's fiber architecture:
We've simplified some parts and left out others (no class components, no legacy context), but the core ideas are the same.
Checklist
anytypes in source codeReady to merge.
Summary by CodeRabbit
Documentation
New Features
Chores
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.