feat: add fiber type guards and assertion utilities#34
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MarcelOlsen
force-pushed
the
fiber/type-guards
branch
from
6fd8dfb to
8839bd3
Compare
MarcelOlsen
force-pushed
the
fiber/types
branch
from
7786ba2 to
791b591
Compare
MarcelOlsen
force-pushed
the
fiber/type-guards
branch
2 times, most recently
from
40c8468 to
922519f
Compare
MarcelOlsen
force-pushed
the
fiber/types
branch
2 times, most recently
from
93ee6bd to
c1702e0
Compare
MarcelOlsen
force-pushed
the
fiber/type-guards
branch
3 times, most recently
from
712f744 to
20e7d76
Compare
Add type predicates for fiber tags (isHostComponent, isFunctionComponent, etc.) and assertion helpers for the fiber tree.
MarcelOlsen
force-pushed
the
fiber/type-guards
branch
from
20e7d76 to
e0d8218
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/fiber/typeGuards.ts`:
- Around line 70-100: Change these boolean-returning helpers to real TypeScript
type guard predicates so the compiler can narrow the Fiber type: update each
function signature (isFunctionComponentFiber, isMemoComponentFiber,
isFragmentFiber, isContextProviderFiber, isContextConsumerFiber) to return a
predicate (for example "fiber is FunctionComponentFiber" or "fiber is Fiber & {
tag: WorkTag.FunctionComponent }" if specific subtype types aren’t defined) and
keep the existing tag comparison bodies unchanged; ensure you import or declare
the narrower subtype names (or use the inline intersection form) so TS can
narrow after calls.
- Around line 302-307: isTextProps currently only checks that props is a
non-null object and that "nodeValue" exists, which allows invalid shapes like {
nodeValue: {} }; update isTextProps to also validate that props.nodeValue is of
the expected TextProps type (string or number) before returning true—keep the
initial null/object guard, then check typeof (props as any).nodeValue ===
"string" || typeof (props as any).nodeValue === "number"; reference the
isTextProps function and TextProps type to locate where to add this stricter
runtime check.
- Around line 31-65: The type guards isHostComponentFiber, isHostTextFiber,
isHostRootFiber and isHostPortalFiber incorrectly assert stateNode is non-null
by only checking fiber.tag; update each guard to also check fiber.stateNode !==
null (e.g., return fiber.tag === WorkTag.HostComponent && fiber.stateNode !==
null) so the narrowing excludes the null case and matches the declared stateNode
union type, preserving safety during mount/unmount phases.
- Around line 451-462: The exported type guards isHTMLElement and
isHTMLInputElement can throw ReferenceError in non-DOM environments; update both
functions to first check typeof HTMLElement !== "undefined" (and typeof
HTMLInputElement !== "undefined" for isHTMLInputElement) before using
instanceof, and return false if those globals are undefined so the guards safely
work in Node/SSR contexts.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/fiber/typeGuards.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MarcelOlsen
Repo: MarcelOlsen/mini-react PR: 8
File: README.md:701-708
Timestamp: 2025-06-17T12:40:39.737Z
Learning: MarcelOlsen prefers to keep test documentation simple with just one example of running specific test files, rather than listing detailed commands for each test suite in the README.
🧬 Code graph analysis (1)
src/fiber/typeGuards.ts (1)
src/fiber/types.ts (11)
Fiber(225-334)FiberRoot(390-443)PortalStateNode(210-212)Hook(170-176)Effect(159-165)UpdateQueue(192-200)Lanes(54-54)Lane(47-47)NoLanes(75-75)Flags(61-61)NoFlags(93-93)
🔇 Additional comments (1)
src/fiber/typeGuards.ts (1)
374-427: Bitwise lane/flag helpers look tight.
Nice, simple, consistent helpers — no drama here.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/fiber/typeGuards.ts`:
- Around line 359-379: assertUpdateQueue currently only checks hook.queue !==
null and can return an object that isn't an UpdateQueue; update it to reuse the
existing isUpdateQueue type guard so it validates the shape. In
assertUpdateQueue(Hook) call isUpdateQueue(hook.queue) and throw a descriptive
Error (e.g., "Hook queue is not a valid UpdateQueue") when the guard fails;
otherwise return hook.queue typed as UpdateQueue<S>. This ensures the function
enforces the UpdateQueue shape rather than just non-null.
- Around line 602-629: The current isMemoComponent only checks $$typeof and
falsely narrows to the full MemoComponent shape; update isMemoComponent to
validate all three properties declared on MemoComponent: confirm "$$typeof"
exists and is a symbol, that "type" exists (e.g., "type" in obj and obj["type"]
!== undefined) and that "compare" exists and is either null or a function
(typeof obj["compare"] === "function" || obj["compare"] === null); keep the same
return type (value is MemoComponent) so downstream code is safe. Ensure you
reference the MemoComponent type and the isMemoComponent function when editing.
- Around line 445-498: isElement and isTextNode access Node.ELEMENT_NODE /
Node.TEXT_NODE directly which can throw in SSR/Node; update both type guards to
first check typeof Node !== "undefined" (same pattern as
isHTMLElement/isHTMLInputElement) so they return false when Node is undefined,
e.g. change isElement to return typeof Node !== "undefined" && node.nodeType ===
Node.ELEMENT_NODE and isTextNode to return typeof Node !== "undefined" &&
node.nodeType === Node.TEXT_NODE; ensure assertElement and assertTextNode still
call these guards (no other changes needed).
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/fiber/typeGuards.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MarcelOlsen
Repo: MarcelOlsen/mini-react PR: 8
File: README.md:701-708
Timestamp: 2025-06-17T12:40:39.737Z
Learning: MarcelOlsen prefers to keep test documentation simple with just one example of running specific test files, rather than listing detailed commands for each test suite in the README.
🧬 Code graph analysis (1)
src/fiber/typeGuards.ts (1)
src/fiber/types.ts (11)
Fiber(225-334)FiberRoot(390-443)PortalStateNode(210-212)Hook(170-176)Effect(159-165)UpdateQueue(192-200)Lanes(54-54)Lane(47-47)NoLanes(75-75)Flags(61-61)NoFlags(93-93)
🔇 Additional comments (8)
src/fiber/typeGuards.ts (8)
27-110: Tag guard block is clean and consistent, chat.
Real predicates, no weirdness. This is exactly the kind of boring, predictable narrowing we want.
116-242: Host fiber assertions are crisp and maintainable.
Error paths are explicit, narrowing is solid, and it’s readable. Nice.
248-298: MemoizedState guards are minimal and tidy.
No surprise casts or spooky action at a distance. Good stuff.
304-353: Props guards look solid.
The runtime checks are aligned with the shape, and the flow is clean.
385-439: Bitmask helpers are fine.
These are simple and do exactly what they say. No notes.
504-524: FiberRoot guard/assert is straightforward.
Simple shape check + assert = good.
530-573: Fiber structural guards are clean.
Clear intent, zero ambiguity.
579-596: Dynamic property helpers are fine for escape hatches.
It’s explicit, and it keeps the scary casts in one place.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/fiber/typeGuards.ts`:
- Around line 362-379: isUpdateQueue currently only checks for keys and can
accept junk; update it to validate the actual required UpdateQueue shape and
types (e.g., ensure "dispatch" is a function, "lanes" is a number, "pending" is
an object or null and any other required fields from the UpdateQueue type such
as the queue's list/head/tail or "last"/"baseState" if present in your type).
Use the UpdateQueue type definition to enumerate and check each mandatory
property and their types inside isUpdateQueue (still returning value is
UpdateQueue<S>); keep assertUpdateQueue<S>(hook: Hook) as-is but rely on the
stronger isUpdateQueue guard to avoid false positives.
- Around line 251-268: The type guards isHookState and isEffectState use partial
checks and must be expanded to validate the full runtime shape: update
isHookState to also check for "baseState" and "baseQueue" properties (in
addition to "memoizedState", "next", "queue") and update isEffectState to also
check for "destroy" and "next" (in addition to "tag", "create", "deps"); ensure
each guard still rejects null/non-objects and uses the same obj alias (value as
Record<string, unknown>) so TypeScript can safely narrow to Hook and Effect
respectively.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/fiber/typeGuards.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MarcelOlsen
Repo: MarcelOlsen/mini-react PR: 8
File: README.md:701-708
Timestamp: 2025-06-17T12:40:39.737Z
Learning: MarcelOlsen prefers to keep test documentation simple with just one example of running specific test files, rather than listing detailed commands for each test suite in the README.
🧬 Code graph analysis (1)
src/fiber/typeGuards.ts (1)
src/fiber/types.ts (11)
Fiber(225-334)FiberRoot(390-443)PortalStateNode(210-212)Hook(170-176)Effect(159-165)UpdateQueue(192-200)Lanes(54-54)Lane(47-47)NoLanes(75-75)Flags(61-61)NoFlags(93-93)
🔇 Additional comments (10)
src/fiber/typeGuards.ts (10)
27-110: Tag guards are clean and actually narrow now.
Yo, predicates + null checks = TS can finally trust these.
116-242: StateNode asserts/readers are tight.
Consistent checks, clear errors, no weird surprises.
274-298: Memoized-state accessors are clean and minimal.
No drama here.
304-353: Props guards/readers look solid.
Nice to see the realTextPropscheck instead of “trust me bro.”
385-439: Lanes/flags helpers are crisp and consistent.
Bitwise ops are clean and readable.
445-498: DOM guards are SSR‑safe and consistent.
Good defensive checks — no ReferenceError landmines.
533-573: Fiber relationship guards are straightforward and clear.
No notes.
579-596: Dynamic property helpers are fine.
Simple and focused.
602-635: Component guards are sane.
The memo shape check is finally legit.
504-524: 🧹 Nitpick | 🔵 TrivialDRY this: FiberRoot guards already exist in
src/fiber/types.ts.
Duplicate implementations drift over time. Consider re‑exporting or delegating to the canonical versions.♻️ Delegate to the existing implementations
-import { NoFlags, NoLanes, WorkTag } from "./types"; +import { + NoFlags, + NoLanes, + WorkTag, + isFiberRoot as isFiberRootImpl, + assertFiberRoot as assertFiberRootImpl, +} from "./types"; @@ -export function isFiberRoot(value: unknown): value is FiberRoot { - if (value === null || typeof value !== "object") { - return false; - } - const obj = value as Record<string, unknown>; - return "containerInfo" in obj && "current" in obj && "pendingLanes" in obj; -} +export const isFiberRoot = isFiberRootImpl; @@ -export function assertFiberRoot(value: unknown): asserts value is FiberRoot { - if (!isFiberRoot(value)) { - throw new Error("Expected FiberRoot"); - } -} +export const assertFiberRoot = assertFiberRootImpl;Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- isUpdatedQueue now vlaidates all five UpdateQueue<S> fields - isHookState now checks all five Hook properties
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/fiber/typeGuards.ts`:
- Around line 602-619: The current helpers setDynamicProperty and
getDynamicProperty allow assigning user-controlled keys like "__proto__",
"constructor", or "prototype", which can cause prototype pollution; update both
functions to explicitly reject unsafe keys (at minimum "__proto__",
"constructor", "prototype") by throwing or returning early when such keys are
passed, or change their names to indicate they are unsafe (e.g.,
setDynamicPropertyUnsafe/getDynamicPropertyUnsafe) if you don't want to add
guards; reference the functions setDynamicProperty and getDynamicProperty when
making this change so callers can be updated or audited accordingly.
- Around line 527-536: The current isFiberRoot type guard only checks for key
existence; tighten it by verifying the types of those keys: ensure
obj.containerInfo is non-null and object-ish, obj.current is non-null and
object-ish, and obj.pendingLanes is a number (use typeof checks and null checks)
before returning true. Update the isFiberRoot function to replace the simple
"'containerInfo' in obj && 'current' in obj && 'pendingLanes' in obj" with
explicit checks for obj.containerInfo !== null && typeof obj.containerInfo ===
"object" && obj.current !== null && typeof obj.current === "object" && typeof
obj.pendingLanes === "number".
- Around line 359-363: The error thrown in assertTextProps incorrectly always
says "Expected TextProps in fiber.pendingProps" even though you check const
props = fiber.pendingProps ?? fiber.memoizedProps; update the check so the
thrown Error names the actual source (pendingProps or memoizedProps) used —
e.g., determine which prop source was selected (by checking if
fiber.pendingProps is undefined) and include that identifier in the Error
message when isTextProps(props) is false; reference assertTextProps,
fiber.pendingProps, fiber.memoizedProps, isTextProps, and TextProps when making
the change.
- Around line 251-279: The current isHookState and isEffectState only check for
property presence and accept invalid shapes; update both guards to validate
basic types for key fields: in isHookState ensure "next" is either null or an
object, and that "queue" and "baseQueue" are either null or objects (and that
"memoizedState"/"baseState" are present but may be any), and in isEffectState
ensure "tag" is a number, "create" is a function, "destroy" is either null or a
function, "deps" is either null or an array, and "next" is either null or an
object; keep the existing property checks but add these minimal
typeof/Array.isArray/null checks for the symbols
"memoizedState","baseState","baseQueue","queue","next","tag","create","destroy","deps"
inside isHookState and isEffectState respectively.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/fiber/typeGuards.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MarcelOlsen
Repo: MarcelOlsen/mini-react PR: 8
File: README.md:701-708
Timestamp: 2025-06-17T12:40:39.737Z
Learning: MarcelOlsen prefers to keep test documentation simple with just one example of running specific test files, rather than listing detailed commands for each test suite in the README.
🧬 Code graph analysis (1)
src/fiber/typeGuards.ts (1)
src/fiber/types.ts (11)
Fiber(225-334)FiberRoot(390-443)PortalStateNode(210-212)Hook(170-176)Effect(159-165)UpdateQueue(192-200)Lanes(54-54)Lane(47-47)NoLanes(75-75)Flags(61-61)NoFlags(93-93)
🔇 Additional comments (7)
src/fiber/typeGuards.ts (7)
31-110: Clean tag guards with real predicates—nice.
Yo, these are actual TS predicates now and the tag checks are straight to the point. This is the kind of boring correctness I like.
116-242: Host fiber asserts are tight and readable.
Tag check + null check + clear error = good guard rails. This is solid.
371-402: UpdateQueue guard is finally legit.
Nice: you validate the core fields + function/null checks, andassertUpdateQueuereuses it.
408-462: Branded lane/flag helpers are clean.
These wrappers keep bitwise ops from leakingnumbereverywhere. 👍
468-521: DOM guards are SSR-safe—good call.
Thetypeofchecks preventReferenceErrorin non-DOM envs, and the asserts reuse the guards. Solid.
553-596: Fiber relationship guards are simple and correct.
Null checks are all you need here, and the assertions are consistent.
625-658: Memo component guard is full-shape now—nice.
You’re checking$$typeof,type, andcompare, andisFunctionTypestays clean and obvious.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- setDynamicProperty / getDynamicProperty, both now reject __proto__, constructor, and prototype keys by throwing an error, preventing prototype pollution - isFiberRoot now validates types: containerInfo must be a non-null object - assertTextProps error message now correctly names the actual source (pendingProps or memoizedProps) based on which one was used - isHookState now validates that baseQueue, queue, and next are each either null or an object - isEffectState now validates that tag is a number, create is a function, destroy is undefined or a function, deps is null or an array, and next is null or an object.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/fiber/typeGuards.ts`:
- Around line 31-65: Replace the explicit inline return types in the type guards
with the existing Host*Fiber aliases to centralize the contract; update
isHostComponentFiber, isHostTextFiber, isHostRootFiber, and isHostPortalFiber to
return the corresponding HostComponentFiber, HostTextFiber, HostRootFiber, and
HostPortalFiber types (e.g., "fiber is HostComponentFiber") while keeping the
same tag checks (WorkTag.HostComponent, WorkTag.HostText, WorkTag.HostRoot,
WorkTag.HostPortal) and stateNode null checks so the narrowed tag and stateNode
types come from the shared aliases.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/fiber/typeGuards.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MarcelOlsen
Repo: MarcelOlsen/mini-react PR: 8
File: README.md:701-708
Timestamp: 2025-06-17T12:40:39.737Z
Learning: MarcelOlsen prefers to keep test documentation simple with just one example of running specific test files, rather than listing detailed commands for each test suite in the README.
🧬 Code graph analysis (1)
src/fiber/typeGuards.ts (1)
src/fiber/types.ts (11)
Fiber(225-334)FiberRoot(390-443)PortalStateNode(210-212)Hook(170-176)Effect(159-165)UpdateQueue(192-200)Lanes(54-54)Lane(47-47)NoLanes(75-75)Flags(61-61)NoFlags(93-93)
🔇 Additional comments (11)
src/fiber/typeGuards.ts (11)
70-110: Tag guards are clean and properly typed.
Nice, tight predicates for the non-host tags.
116-242: Assertions + Host*Fiber types are solid.
Clear error messages and explicit narrowing — good stuff.
248-318: Memoized-state guards look tight.
Minimal-but-real shape checks; nice balance.
324-375: Props guards are straightforward and readable.
nodeValuecheck is the right call for runtime narrowing.
381-412: UpdateQueue guard + assertion is clean.
Good to see the assertion route through the guard.
418-472: Bitwise helpers are concise and well-named.
“Unsafe” labeling is a nice cue to callers.
478-531: DOM guards are SSR-safe — nice.
Thetypeofchecks are the right defensive move.
537-563: FiberRoot guard is properly tightened.
Good shape validation without overfitting.
569-612: Fiber structure guards are clean and predictable.
Exactly what you want here.
618-645: Prototype-pollution defenses are on point.
UNSAFE_KEYSis a solid, centralized guard.
651-684: Component type guards look good.
isMemoComponentshape checks are tight and practical.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/fiber/typeGuards.ts`:
- Around line 245-281: The isHookState guard currently accepts any non-null
object for "queue" and "baseQueue"; tighten it by validating those with
isUpdateQueue when non-null. Update isHookState to: import/ensure isUpdateQueue
is in scope and replace the checks (obj["baseQueue"] === null || typeof
obj["baseQueue"] === "object") and (obj["queue"] === null || typeof obj["queue"]
=== "object") with (obj["baseQueue"] === null ||
isUpdateQueue(obj["baseQueue"])) and (obj["queue"] === null ||
isUpdateQueue(obj["queue"])); keep the other property checks and return shape
unchanged.
- Around line 337-369: The error message in assertTextProps can report the wrong
source when fiber.pendingProps is null because the code uses nullish coalescing
(??) but checks only for undefined; update the source detection in
assertTextProps to use a nullish check (e.g., check pendingProps !== null &&
pendingProps !== undefined or pendingProps == null) so the computed source
("pendingProps" vs "memoizedProps") correctly reflects which prop was used when
throwing the Error in assertTextProps.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/fiber/typeGuards.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MarcelOlsen
Repo: MarcelOlsen/mini-react PR: 8
File: README.md:701-708
Timestamp: 2025-06-17T12:40:39.737Z
Learning: MarcelOlsen prefers to keep test documentation simple with just one example of running specific test files, rather than listing detailed commands for each test suite in the README.
🧬 Code graph analysis (1)
src/fiber/typeGuards.ts (1)
src/fiber/types.ts (11)
Fiber(225-334)FiberRoot(390-443)PortalStateNode(210-212)Hook(170-176)Effect(159-165)UpdateQueue(192-200)Lanes(54-54)Lane(47-47)NoLanes(75-75)Flags(61-61)NoFlags(93-93)
🔇 Additional comments (16)
src/fiber/typeGuards.ts (16)
31-59: Host fiber guards look tight.
Tag + non-null stateNode checks are clean and consistent.
61-104: Tag-only fiber guards are clean.
Straightforward and readable.
110-140: Host*Fiber aliases are solid.
Centralized contracts = easier maintenance.
147-204: Assertions are crisp with useful failure messages.
No notes here.
210-236: Host stateNode helpers are straightforward.
Looks good.
288-312: Memoized state helpers are clean.
All good.
318-332: TextProps guard is properly strict.
Nice and simple.
378-406: UpdateQueue guard + assert are solid.
No issues.
415-466: Lane/flag helpers are clean and consistent.
Ship it.
475-503: DOM guards are SSR-safe and tidy.
Good defensive checks.
510-525: DOM asserts are straight to the point.
No notes.
534-557: FiberRoot guard shape checks look good.
Solid runtime narrowing.
566-606: Fiber relationship guards are clean.
Nice and minimal.
612-639: Dynamic property helpers are hardened — good call.
Prototype pollution defense is on point.
648-669: MemoComponent guard looks complete.
Checks line up with the type.
674-678: Function-type guard is fine.
No issues.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/fiber/typeGuards.ts`:
- Around line 531-557: Remove the duplicate isFiberRoot and assertFiberRoot
implementations here and instead import and re-export the canonical definitions
from the other module (the existing isFiberRoot and assertFiberRoot in
src/fiber/types.ts), or move the single authoritative implementation into this
module and update types.ts to re-export it; ensure the current symbols
isFiberRoot and assertFiberRoot remain available from this file by using a
re-export so callers keep working and you avoid duplicate/ drifting
implementations.
- Around line 245-258: The isHookState type guard wrongly validates
Hook.baseQueue as UpdateQueue via isUpdateQueue, but Hook.baseQueue is Update |
null; update the check in isHookState to accept null or an Update (either by
replacing isUpdateQueue with an isUpdate guard if available, or by loosening the
check to only assert null or an object shaped like an Update) so legitimate
hooks don't fail assertHookState; refer to the isHookState function, the Hook
type, the baseQueue property, and replace the (obj["baseQueue"] === null ||
isUpdateQueue(obj["baseQueue"])) check accordingly.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/fiber/typeGuards.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MarcelOlsen
Repo: MarcelOlsen/mini-react PR: 8
File: README.md:701-708
Timestamp: 2025-06-17T12:40:39.737Z
Learning: MarcelOlsen prefers to keep test documentation simple with just one example of running specific test files, rather than listing detailed commands for each test suite in the README.
🧬 Code graph analysis (1)
src/fiber/typeGuards.ts (1)
src/fiber/types.ts (11)
Fiber(225-334)FiberRoot(390-443)PortalStateNode(210-212)Hook(170-176)Effect(159-165)UpdateQueue(192-200)Lanes(54-54)Lane(47-47)NoLanes(75-75)Flags(61-61)NoFlags(93-93)
🔇 Additional comments (3)
src/fiber/typeGuards.ts (3)
31-237: Solid guard + assert layering.
This is clean: tag guards feed into assert helpers, and the Host*Fiber aliases keep the contracts consistent. Good, tight surface.
318-466: Props + UpdateQueue + lane/flag helpers look tight.
Nice runtime checks and the branded bitwise helpers are straightforward and consistent.
472-525: DOM + dynamic-property + memo guards are in good shape.
SSR-safe DOM checks are solid, and the unsafe key block is exactly the kind of hardening I want to see.Also applies to: 612-678
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/fiber/typeGuards.ts`:
- Around line 300-313: The two helpers getMemoizedState<T> and
assertMemoizedState<T> perform unchecked casts and can mislead downstream users
into thinking they validate the shape; update their JSDoc comments to explicitly
warn that these functions do not perform runtime validation of T and recommend
using specific guards (for example mention isHookState) when validation is
required, and ensure the assertMemoizedState doc notes it throws if
memoizedState is null; keep the implementation unchanged but add those
clarifying sentences to the JSDoc for both functions.
- Around line 114-141: Move the Host* type aliases so they appear before the
type guard functions that reference them: place HostComponentFiber,
HostTextFiber, HostRootFiber, and HostPortalFiber above the type guard
implementations (e.g., the functions that use the "fiber is HostComponentFiber"
style predicates) so the file reads top-to-bottom; simply cut the four type
alias blocks and paste them above the corresponding isHost... type guard
functions to improve readability and maintainability.
- Around line 437-456: The three functions mergeLanesUnsafe,
intersectLanesUnsafe, and removeLanesUnsafe should be renamed to remove the
misleading "Unsafe" suffix (e.g., mergeLanes, intersectLanes, removeLanes) and
their JSDoc updated to reflect they perform bitwise ops while handling branded
Lane/Lanes types; update all references/exports/usages across the codebase to
the new names so imports and calls continue to work, and keep the current
implementations and signatures intact (only change the identifier names and
docs).
- Around line 403-420: The isUpdateQueue type guard currently only checks that
obj["pending"] is null or an object; update it to validate pending using the
existing isUpdate type guard (i.e., require obj["pending"] === null ||
isUpdate(obj["pending"])) so pending is fully validated as an Update<S> when
present; modify the isUpdateQueue function accordingly and keep the other field
checks (lanes, dispatch, lastRenderedReducer, lastRenderedState) unchanged.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/fiber/typeGuards.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MarcelOlsen
Repo: MarcelOlsen/mini-react PR: 8
File: README.md:701-708
Timestamp: 2025-06-17T12:40:39.737Z
Learning: MarcelOlsen prefers to keep test documentation simple with just one example of running specific test files, rather than listing detailed commands for each test suite in the README.
🧬 Code graph analysis (1)
src/fiber/typeGuards.ts (1)
src/fiber/types.ts (12)
Fiber(225-334)FiberRoot(390-443)PortalStateNode(210-212)Hook(170-176)Effect(159-165)Update(181-187)UpdateQueue(192-200)Lanes(54-54)Lane(47-47)NoLanes(75-75)Flags(61-61)NoFlags(93-93)
🔇 Additional comments (11)
src/fiber/typeGuards.ts (11)
32-60: DUDE, LOOK AT THESE TYPE GUARDS NOW! Actually narrowing types like a BOSS.The previous feedback about these being fake type guards has been addressed. Now we've got proper
fiber is HostComponentFiberpredicates that TypeScript can actually use for narrowing. This is the way.The stateNode !== null checks combined with tag checks means downstream code knows EXACTLY what it's dealing with. Clean.
148-237: These assertion functions are CHEF'S KISS.Proper
asserts fiber is Xreturn types, descriptive error messages with the actual tag value, separate checks for tag and stateNode. This is how you do runtime validation that TypeScript can actually leverage.
assertHostFiberhandling both HostComponent and HostText is a nice ergonomic touch for call sites that don't care which host type they have.
246-261: baseQueue validation is FIXED - using isUpdate now instead of isUpdateQueue.Past review caught that
Hook.baseQueueisUpdate | null, notUpdateQueue. This is now correctly validating withisUpdate(obj["baseQueue"])at line 255. The type contract is no longer a lie. Good fix.
327-333: isTextProps actually validates the type now. No more "nodeValue: {}" sneaking through!Previous feedback about this being a false type guard has been addressed. Now we check
typeof nv === "string" || typeof nv === "number"which means the contract is actually enforced at runtime. TypeScript narrowing is no longer lying.
362-370: Error source detection using!= null- FIXED.Past review caught the bug where
!== undefinedwouldn't catchnullpendingProps. Now using!= null(loose equality) correctly handles both null and undefined cases. The error message will accurately report which prop source was used.
426-431: assertUpdateQueue using isUpdateQueue now - FIXED!Past review caught that this was only checking
hook.queue === nulland not validating shape. Now it properly usesisUpdateQueue<S>(hook.queue)to ensure the queue is actually valid before returning. The assertion means something now.
500-528: SSR-safe DOM guards - typeof checks before accessing globals. LETSGOOOO!Previous feedback about
ReferenceErrorin Node.js environments has been addressed:
- Line 501:
typeof Node !== "undefined" && node.nodeType === Node.ELEMENT_NODE- Line 508:
typeof Node !== "undefined" && node.nodeType === Node.TEXT_NODE- Line 515:
typeof HTMLElement !== "undefined" && node instanceof HTMLElement- Line 525:
typeof HTMLInputElement !== "undefined" && element instanceof HTMLInputElementShort-circuit evaluation means the globals are never accessed if they don't exist. This is ready for SSR (Phase 17 on the roadmap).
591-631: Simple but effective navigation guards. Love the ergonomics.These are clean type predicates that narrow
Fiber | nulltoFiber. The patternfiber is Fiber & { return: Fiber }is exactly right.
assertFiberParentreturning the parent value is smart - lets you doconst parent = assertFiberParent(fiber)instead of separate assert + access.
637-664: Prototype pollution protection is IN. Set-based O(1) lookup is clean.Previous feedback about
__proto__/constructor/prototypeinjection has been addressed. Both setter and getter now throw on unsafe keys - that's actually better than silently returning undefined because it surfaces misuse immediately.The
UNSAFE_KEYSSet at line 637 is efficient and easy to extend if needed.
682-694: isMemoComponent validates the FULL shape now. No more half-truths.Previous feedback addressed - now checking:
typeof obj["$$typeof"] === "symbol""type" in obj && obj["type"] !== undefinedtypeof obj["compare"] === "function" || obj["compare"] === nullIf someone passes
{ $$typeof: Symbol() }withouttypeorcompare, it correctly returns false. The type guard matches theMemoComponenttype contract.
559-571: Type validation is actually solid - no{ pendingLanes: "nope" }slipping through hereThe
isFiberRootguard is doing its job right:
containerInfocheck: non-null AND object ✓currentcheck: non-null AND object ✓pendingLanescheck: number type ✓And
assertFiberRootcomposition is clean - leans onisFiberRootrather than duplicating logic. Good architectural instinct there.Also checked
src/fiber/types.ts- no duplicates exist. That past concern was a ghost. File maintains consistent pattern across all its guards, which is maintainable.
…eanup
- Convert boolean-returning fiber tag helpers to real TS type predicates
- Add stateNode !== null checks to host fiber type guards
- Validate full shapes in isHookState, isEffectState, isUpdateQueue, and
isMemoComponent instead of only checking key existence
- Add isUpdate type guard for Update<S> shape validation
- Tighten isTextProps to validate nodeValue is string or number
- Tighten isFiberRoot to verify property types, not just presence
- Guard isHTMLElement, isHTMLInputElement, isElement, isTextNode against
missing DOM globals for SSR safety
- Fix assertTextProps error message to report correct prop source
- Reject prototype-polluting keys in setDynamicProperty/getDynamicProperty
- Add JSDoc warnings to getMemoizedState/assertMemoizedState about unchecked casts
- Rename mergeLanesUnsafe/intersectLanesUnsafe/removeLanesUnsafe to
mergeLanes/intersectLanes/removeLanes
- Move Host* type aliases above their corresponding type guard functions
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/fiber/typeGuards.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MarcelOlsen
Repo: MarcelOlsen/mini-react PR: 8
File: README.md:701-708
Timestamp: 2025-06-17T12:40:39.737Z
Learning: MarcelOlsen prefers to keep test documentation simple with just one example of running specific test files, rather than listing detailed commands for each test suite in the README.
🧬 Code graph analysis (1)
src/fiber/typeGuards.ts (1)
src/fiber/types.ts (12)
Fiber(225-334)FiberRoot(390-443)PortalStateNode(210-212)Hook(170-176)Effect(159-165)Update(181-187)UpdateQueue(192-200)Lanes(54-54)Lane(47-47)NoLanes(75-75)Flags(61-61)NoFlags(93-93)
🔇 Additional comments (11)
src/fiber/typeGuards.ts (11)
31-58: Host fiber aliases are clean and readable.*
This is tidy and consistent.
98-141: Tag-only guards (Function/Memo/Fragment/Context) are fine.
No notes here.
152-241: Assertions are consistent and the errors read well.
This is clean and usable.
250-323: Hook/effect guards + memoized helpers look solid.
Nice balance of runtime checks vs. documented casts.
329-380: Props/TextProps handling is tight.
Nullish source selection + type check is on point.
436-502: UpdateQueue assert + lane/flag helpers are clean.
Straightforward, consistent, no weirdness.
507-560: DOM guards are SSR-safe; nice.
Good defensive checks.
569-642: FiberRoot + parent/child guards are straightforward.
Clear and simple.
647-674: Prototype-pollution key guard is good safety.
Nice hard stop.
680-713: MemoComponent guard + isFunctionType are tidy.
Looks good.
389-429: Hold up, the fields are all there, chief.
Just ran the verification and all those fields actually exist in the realUpdateandUpdateQueuetypes in src/fiber/types.ts—lane, action, hasEagerState, eagerState, next for Update, and pending, lanes, dispatch, lastRenderedReducer, lastRenderedState for UpdateQueue. So legit data won't get rejected by these guards due to missing fields.That said, while you're checking field existence, you're not type-validating everything you should be—
actionandeagerStatein Update, andlastRenderedStatein UpdateQueue don't get type-checked, just existence-checked. That's sus for a type guard that's supposed to narrow the type. But that's a different issue than what you originally flagged.Likely an incorrect or invalid review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fiber/typeGuards.ts`:
- Around line 68-96: The current type guards (isHostComponentFiber,
isHostTextFiber, isHostRootFiber, isHostPortalFiber) only check tag + non-null —
tighten them to validate the actual shape of stateNode by calling the existing
runtime validators instead of just checking !== null; e.g. in
isHostComponentFiber call the host element validator (e.g.,
isHostElementStateNode(stateNode)), in isHostTextFiber call the text-instance
validator (e.g., isTextStateNode(stateNode)), in isHostRootFiber call the
fiber-root validator (e.g., isFiberRoot(stateNode)), and in isHostPortalFiber
call the portal-state validator (e.g., isPortalStateNode(stateNode)) so each
function returns tag-check && stateNode !== null &&
correspondingValidator(stateNode).
1 participant
Add type predicates for fiber tags (isHostComponent, isFunctionComponent,
etc.) and assertion helpers for the fiber tree.
Summary by CodeRabbit
Release Notes