Conversation
|
Warning Rate limit exceeded@MarcelOlsen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAlright, chat, this PR is giga-chad level: it slaps a full context API onto MiniReact, wires up the reconciler for context propagation, and drops a massive README overhaul with a split roadmap (alpha/stable). There’s also a new context test suite, and the package scripts get a proper biome glow-up. Absolute banger of a change. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Provider
participant Consumer
participant MiniReact
participant Reconciler
participant ContextStack
App->>MiniReact: render(<Provider value={...}><Consumer /></Provider>)
MiniReact->>Reconciler: createVDOMInstance(Provider)
Reconciler->>ContextStack: pushContext(contextId, value)
Reconciler->>Reconciler: reconcile children (Consumer)
Reconciler->>MiniReact: useContext(contextId)
MiniReact->>ContextStack: getCurrentValue(contextId)
Reconciler->>ContextStack: popContext()
MiniReact->>App: render output with context value
Possibly related PRs
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
src/MiniReact.ts (1)
349-365: Leverage default on the context object itself
useContextrecomputes the default via_defaultValue, ignoring the (possibly newer)_currentValue.
If you keep_currentValue, lean on it here; if not, delete both and stay single-source-of-truth.src/types.ts (2)
94-108:ContextHookis unused – dead code is dead weightI don’t see it referenced in the runtime. Yank it or wire it up so the type system actually earns its keep.
141-146: Two fields for one truth? Pick one 🔥
_currentValuevs_defaultValue– unless you’re updating_currentValue, this duality just confuses.
Simplify to a singledefaultValueor keep them in sync.README.md (3)
75-80: Next milestones look solid—use en dashes!
Love the clarity on Phases 9–11 and the v0.1.0 alpha goal. Quick nit: for time ranges, swap your hyphens for en dashes (2–3 weeks,1–2 weeks) to level up typography.
82-82: Grammar nit: add “the”
Consider writing “the stable v1.0.0 release” for smoother reading.
685-695: Upgrade that test suite heading
That bold line for the comprehensive tests is screaming for a proper heading level and those trailing colons on “Test Categories” and “Running Tests” will trip Markdown lint. Swap to headings and drop the punctuation.- **Comprehensive Test Suite: Unit and integration tests across 8 files** + ### Comprehensive Test Suite - ### Test Categories: + ### Test Categories - ### Running Tests: + ### Running Tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(13 hunks)package.json(1 hunks)src/MiniReact.ts(6 hunks)src/reconciler.ts(8 hunks)src/types.ts(2 hunks)tests/MiniReact.context.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/MiniReact.ts (2)
src/reconciler.ts (1)
setContextHooks(23-29)src/types.ts (4)
MiniReactContext(141-146)FunctionalComponent(9-11)AnyMiniReactElement(7-7)VDOMInstance(127-135)
🪛 markdownlint-cli2 (0.17.2)
README.md
15-15: Link fragments should be valid
null
(MD051, link-fragments)
20-20: Link fragments should be valid
null
(MD051, link-fragments)
21-21: Link fragments should be valid
null
(MD051, link-fragments)
24-24: Link fragments should be valid
null
(MD051, link-fragments)
25-25: Link fragments should be valid
null
(MD051, link-fragments)
26-26: Link fragments should be valid
null
(MD051, link-fragments)
27-27: Link fragments should be valid
null
(MD051, link-fragments)
28-28: Link fragments should be valid
null
(MD051, link-fragments)
32-32: Link fragments should be valid
null
(MD051, link-fragments)
38-38: Link fragments should be valid
null
(MD051, link-fragments)
685-685: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
687-687: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
698-698: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 LanguageTool
README.md
[typographical] ~80-~80: If specifying a range, consider using an en dash instead of a hyphen.
Context: ... 🎉 Alpha Release v0.1.0: Target in 4-6 weeks Post-Alpha Roadmap: 12 addit...
(HYPHEN_TO_EN)
[uncategorized] ~82-~82: You might be missing the article “the” here.
Context: ...map**: 12 additional phases planned for stable v1.0.0 release with advanced features i...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~418-~418: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... - Priority-based scheduling - High/low priority updates (similar to React's concurrent ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~445-~445: This phrase is redundant (‘O’ stands for ‘optimization’). Use simply “SEO”.
Context: ...that work on both client and server - SEO optimization - Meta tag management and structured ...
(ACRONYM_TAUTOLOGY)
🔇 Additional comments (5)
package.json (1)
10-16: Scripts look clean, ship it 🚀Nothing to nit on these
biomeshortcuts – they read well and do exactly what they say.
Keep crushing it.tests/MiniReact.context.test.ts (1)
1-952: Legendary test coverage, chat!This suite hammers every edge-case I could imagine. Absolute chef’s kiss.
README.md (3)
184-186: Effects & Context features – rock on!
Fantastic call including the Effects System and Context API in your Completed Features. This matches the code changes to the context system perfectly.
15-38: Alright, squad—our last script got bodied by sed’s brackets and a missingcomm, so let’s drop a simpler, hard-hitting check:#!/usr/bin/env bash # 🔨 Extract all TOC link anchors rg -oP '\(#\K[^)]+' README.md | sort > toc_links.txt # 🔨 Extract all headings, normalize to GitHub-style slugs rg '^#+ ' README.md \ | sed -E 's/ *\[#.*\]//; s/[^a-zA-Z0-9 -]//g; s/ +/-/g; s/--+/-/g; s/^-+|-+$//g' \ | tr '[:upper:]' '[:lower:]' \ | sort > headings.txt # 🔨 Show any anchors that don’t match real headings grep -Fxv -f headings.txt toc_links.txt || echo "All anchors match headings 🎉"
63-73: Let’s suss out what test runner you’re packing in—gotta know how to call the suite, baby! 🔍✨#!/usr/bin/env bash # Peek the devDependencies to spot our test runner jq '.devDependencies' package.json
| // Set the context hooks in the reconciler | ||
| setContextHooks( | ||
| (contextValues: Map<symbol, unknown>) => { | ||
| contextStack.push(contextValues); | ||
| }, | ||
| () => { | ||
| contextStack.pop(); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Wrap push/pop in a try/finally, my dude 🔥
If any child reconciliation throws, we’ll leave the contextStack in a dirty state – doom-spiral for every render after that.
Guard the push with a try and guarantee the pop() in finally.
🤖 Prompt for AI Agents
In src/MiniReact.ts around lines 62 to 70, the contextStack push and pop
operations are not protected against exceptions during child reconciliation,
which can leave the stack in an inconsistent state. Wrap the push call in a try
block and ensure the pop call happens in a finally block to guarantee the stack
is cleaned up even if an error occurs during reconciliation.
MarcelOlsen
force-pushed
the
integration/context-system
branch
from
bbc52de to
44905ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/reconciler.ts (2)
163-176: Context stack still left dangling – wrap push/pop in atry…finally, chat!We’ve literally been here before: if the functional component blows up after
pushContextFunction,popContextFunctionnever fires and your global context stack is yeeted into oblivion.
Same safety-net we pleaded for last round, still MIA. Slam atryaround the child reconciliation and callpop…in thefinally. EZ clap.- if (instance.contextValues && pushContextFunction) { - pushContextFunction(instance.contextValues); - } - - const childInstance = childElement - ? createVDOMInstance(parentDom, childElement) - : null; - - // Pop context AFTER reconciling children - if (instance.contextValues && popContextFunction) { - popContextFunction(); - } + let pushed = false; + if (instance.contextValues && pushContextFunction) { + pushContextFunction(instance.contextValues); + pushed = true; + } + let childInstance: VDOMInstance | null = null; + try { + childInstance = childElement + ? createVDOMInstance(parentDom, childElement) + : null; + } finally { + if (pushed && popContextFunction) { + popContextFunction(); + } + }
535-550: Déjà vu inupdateVDOMInstance– still nofinally, still riskyCopy-pasta the exact same push/pop pattern into updates without a guard? C’mon.
Wrap it or watch the context stack leak harder than my coffee at 3 AM.Same diff pattern as above applies here.
README.md (1)
160-162: Context tests still AWOL in the quick-start – we talked about thisYou added a 1 k-line context suite but the README still only shows the render test.
Give newcomers the one-liner so they can flex the new API.# Run specific test file bun test tests/MiniReact.render.test.ts +# Run context tests +bun test tests/MiniReact.context.test.ts
🧹 Nitpick comments (1)
tests/MiniReact.context.test.ts (1)
780-806: Triple-cast madness – ditch theas unknown askaraoke
(exposedSetContextValue as unknown as (value: …) => void)shows up all over.
That’s TypeScript screaming “help me”. Capture the setter with a properly typed ref instead of unsafe double-casting.Example:
let setCtx!: (v: string | number | boolean) => void; const App: FunctionalComponent = () => { const [ctx, setCtxFn] = useState<...>("initial"); setCtx = setCtxFn; ... };Cleaner, safer, zero casts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(13 hunks)package.json(1 hunks)src/MiniReact.ts(6 hunks)src/reconciler.ts(8 hunks)src/types.ts(2 hunks)tests/MiniReact.context.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- src/types.ts
- src/MiniReact.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/MiniReact.context.test.ts (1)
src/MiniReact.ts (3)
createContext(302-342)useContext(349-365)useState(157-217)
🪛 markdownlint-cli2 (0.17.2)
README.md
15-15: Link fragments should be valid
null
(MD051, link-fragments)
20-20: Link fragments should be valid
null
(MD051, link-fragments)
21-21: Link fragments should be valid
null
(MD051, link-fragments)
24-24: Link fragments should be valid
null
(MD051, link-fragments)
25-25: Link fragments should be valid
null
(MD051, link-fragments)
26-26: Link fragments should be valid
null
(MD051, link-fragments)
27-27: Link fragments should be valid
null
(MD051, link-fragments)
28-28: Link fragments should be valid
null
(MD051, link-fragments)
32-32: Link fragments should be valid
null
(MD051, link-fragments)
38-38: Link fragments should be valid
null
(MD051, link-fragments)
685-685: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
687-687: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
698-698: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 LanguageTool
README.md
[typographical] ~80-~80: If specifying a range, consider using an en dash instead of a hyphen.
Context: ... 🎉 Alpha Release v0.1.0: Target in 4-6 weeks Post-Alpha Roadmap: 12 addit...
(HYPHEN_TO_EN)
[uncategorized] ~82-~82: You might be missing the article “the” here.
Context: ...map**: 12 additional phases planned for stable v1.0.0 release with advanced features i...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~418-~418: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... - Priority-based scheduling - High/low priority updates (similar to React's concurrent ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~445-~445: This phrase is redundant (‘O’ stands for ‘optimization’). Use simply “SEO”.
Context: ...that work on both client and server - SEO optimization - Meta tag management and structured ...
(ACRONYM_TAUTOLOGY)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/reconciler.ts (1)
420-436: We’re yeeting DOM nodes but ghost-leaving their hooks – memory leak alert 🟥Left-over keyed / unkeyed children get their DOM brutally
removeDomNode-d, but their effect hooks never see a cleanup pass, nor do their own children get traversed. The reconcile call you used at the top (reconcile(null, null, childInstance)) already does the full unmount dance (effect cleanup, recursive descent, eventSystem unregister, etc.).Let’s use the same single source of truth instead of re-implementing (and partially forgetting) teardown logic.
- // Remove any remaining old keyed children that weren't reused - for (const [, oldChild] of oldKeyed) { - if (oldChild.dom) { - eventSystem.unregisterInstance(oldChild); - removeDomNode(oldChild.dom); - } - } + // Unmount any remaining keyed children + for (const [, oldChild] of oldKeyed) { + reconcile(null, null, oldChild); // full teardown incl. hooks + } - // Remove any remaining old unkeyed children that weren't reused - for (let i = unkeyedIndex; i < oldUnkeyed.length; i++) { - const oldChild = oldUnkeyed[i]; - if (oldChild.dom) { - eventSystem.unregisterInstance(oldChild); - removeDomNode(oldChild.dom); - } - } + // …and the unkeyed stragglers + for (let i = unkeyedIndex; i < oldUnkeyed.length; i++) { + reconcile(null, null, oldUnkeyed[i]); + }Without this, every un-reused subtree keeps its effects and memory around forever. Not pog.
🧹 Nitpick comments (3)
src/reconciler.ts (3)
548-560: DRY it up – push/try/finally/pop pattern is copy-pastedYou duplicated the exact same push-context / try / finally / pop sequence in both
createVDOMInstanceandupdateVDOMInstance. Extract a tiny helper (e.g.withProviderContext(instance, fn)) so we can’t accidentally drift later. Makes the reconciler easier to scan at 3 AM.
233-234: Prettier is screaming – kill the stray spacesPipeline failed because of the extra space inside the empty block.
-let setCurrentRenderInstance: (instance: VDOMInstance | null) => void = () => { }; +let setCurrentRenderInstance: (instance: VDOMInstance | null) => void = () => {};One tiny tweak, CI goes green.
550-554: Multiline call vs Prettier – single-liner winsPrettier failure #2. Inline the call, no readability lost:
-childInstance = reconcile( - parentNode, - childElement, - oldChildInstance, -); +childInstance = reconcile(parentNode, childElement, oldChildInstance);Ship it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/MiniReact.ts(7 hunks)src/reconciler.ts(9 hunks)tests/MiniReact.context.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MiniReact.ts
🧰 Additional context used
🪛 GitHub Actions: CI
src/reconciler.ts
[error] 234-234: Prettier formatting check failed. Formatter would have changed 'let setCurrentRenderInstance: (instance: VDOMInstance | null) => void = () => { };' spacing.
[error] 550-554: Prettier formatting check failed. Formatter would have replaced multiline reconcile call with a single line call: 'childInstance = reconcile(parentNode, childElement, oldChildInstance);'.
🔇 Additional comments (1)
tests/MiniReact.context.test.ts (1)
26-34: Tests poke internals – future refactor foot-gun
_defaultValue,_currentValue,_contextIdare undocumented internals. If the implementation ever makes them#privateor moves them behind Symbols, these tests will blow up even though public behaviour is fine. Consider asserting through the public API only (render + read), or expose a sanctionedgetCurrentValue()helper for test envs.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/reconciler.ts (1)
491-498: 🫡 Same leak pattern in the update path – slap thatfinallyon!The update branch repeats the push / call / pop sequence with no
exception-safety. Copy the pattern from the creation path to keep the render
stack pure.
🧹 Nitpick comments (1)
src/reconciler.ts (1)
310-333: 📚 Docstring vs. signature drift indiffPropsThe JSDoc lists an
instanceparam that the function no longer accepts.
Small but confusing – yank the stale mention or pass the param.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/MiniReact.ts(6 hunks)src/reconciler.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MiniReact.ts
| // Find the correct parent node to reconcile in | ||
| let parentNode: Node | null = null; | ||
| if (oldChildInstance?.dom?.parentNode) { | ||
| parentNode = oldChildInstance.dom.parentNode; | ||
| } else if (instance.dom?.parentNode) { | ||
| parentNode = instance.dom.parentNode; | ||
| } | ||
|
|
||
| if (!parentNode) { | ||
| throw new Error( | ||
| "Unable to find parent node for functional component reconciliation", | ||
| ); |
There was a problem hiding this comment.
🤔 “Unable to find parent node” when prev render returned null
parentNode is resolved only from oldChildInstance.dom or instance.dom.
Scenario: component previously rendered null, now returns a tree ⇒ both DOM
refs are null ⇒ we throw.
Walk up via instance.parent (or accept parentDom as a param) before
bailing.
- if (!parentNode) {
- throw new Error(
- "Unable to find parent node for functional component reconciliation",
- );
- }
+ if (!parentNode) {
+ // fallback: climb until we hit a host node
+ let p: VDOMInstance | undefined = instance.parent;
+ while (p && !parentNode) {
+ parentNode = p.dom ?? null;
+ p = p.parent;
+ }
+ if (!parentNode) {
+ throw new Error(
+ "Unable to find parent node for functional component reconciliation",
+ );
+ }
+ }📝 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.
| // Find the correct parent node to reconcile in | |
| let parentNode: Node | null = null; | |
| if (oldChildInstance?.dom?.parentNode) { | |
| parentNode = oldChildInstance.dom.parentNode; | |
| } else if (instance.dom?.parentNode) { | |
| parentNode = instance.dom.parentNode; | |
| } | |
| if (!parentNode) { | |
| throw new Error( | |
| "Unable to find parent node for functional component reconciliation", | |
| ); | |
| // Find the correct parent node to reconcile in | |
| let parentNode: Node | null = null; | |
| if (oldChildInstance?.dom?.parentNode) { | |
| parentNode = oldChildInstance.dom.parentNode; | |
| } else if (instance.dom?.parentNode) { | |
| parentNode = instance.dom.parentNode; | |
| } | |
| if (!parentNode) { | |
| // fallback: climb until we hit a host node | |
| let p: VDOMInstance | undefined = instance.parent; | |
| while (p && !parentNode) { | |
| parentNode = p.dom ?? null; | |
| p = p.parent; | |
| } | |
| if (!parentNode) { | |
| throw new Error( | |
| "Unable to find parent node for functional component reconciliation", | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/reconciler.ts around lines 501 to 512, the code only checks
oldChildInstance.dom.parentNode and instance.dom.parentNode to find the parent
node, which fails if the previous render returned null and both DOM references
are null. To fix this, extend the logic to walk up the component tree using
instance.parent to find a valid parent DOM node before throwing an error.
Alternatively, consider accepting a parentDom parameter to use as a fallback
parent node.
d832d81 to
c6600ee
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary by CodeRabbit
createContextanduseContexthooks for managing shared state.