Walkthrough

Implements deduplication of component renders to prevent hydration mismatches when reactOnRailsPageLoaded() is invoked multiple times. Tracks rendered components in a renderedRoots map, skips re-renders for the same DOM node, unmounts and re-renders when DOM nodes are replaced, and updates the hydration heuristic to activate only when DOM nodes contain server-rendered content.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

• ClientRenderer.ts: Verify correctness of render deduplication logic, unmount lifecycle handling, error handling in unmount operations, and hydration heuristic—ensure no hydration occurs for previously client-rendered content
• Test coverage: Confirm unit and E2E tests exercise the edge cases (same DOM node, replaced DOM node, multiple invocations)
• Integration: Check that test fixtures properly isolate behavior and don't mask real issues

Suggested reviewers

• Judahmeek
• alexeyr-ci
• Romex91

Poem

🐰 Multiple renders? A hydration fright,
Deduplication sets the stage just right—
Same DOM skipped, replaced ones remount with care,
ReactOnRails renders without despair! ✨

Pre-merge checks and finishing touches

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 @coderabbitai help to get the list of available commands and usage tips.}

size-limit report 📦

Code Review Summary

This PR effectively fixes issue #2210 where calling reactOnRailsPageLoaded() multiple times caused hydration mismatch errors for client-rendered components. The solution is elegant and well-tested.

Strengths

1. Root Cause Analysis is Spot-On

The fix correctly identifies that the problem occurs when reactOnRailsPageLoaded() is called multiple times - the shouldHydrate check (!!domNode.innerHTML) was incorrectly treating previously client-rendered content as server-rendered content.

2. Minimal, Surgical Fix

• Only 14 lines changed in production code (ClientRenderer.ts)
• Leverages existing renderedRoots Map infrastructure
• No breaking changes or performance overhead
• Clean early return pattern (lines 84-89)

3. Comprehensive Test Coverage

• Unit test verifies the core fix
• E2E Playwright test validates actual user scenario
• Manual test page provides visual verification
• AsyncComponent uses inline styles to expose hydration mismatches

4. Code Quality

• Clear comments explaining the why
• Consistent trace logging for debugging
• Good test organization referencing issue Hydration mismatch with React 19 and the latest beta version of this gem #2210

Observations and Suggestions

1. Missing Changelog Entry (BLOCKING)

According to CLAUDE.md, user-visible bug fixes should be added to CHANGELOG.md. This is a user-facing fix for a regression.

Suggested addition under Fixed section:

• Fixed hydration mismatch errors when reactOnRailsPageLoaded() called multiple times. PR 2211 by justin808. Fixes Hydration mismatch with React 19 and the latest beta version of this gem #2210.

2. E2E Test Status

The PR description shows unchecked Playwright E2E test items. Please confirm these are passing in CI.

3. Potential Edge Case

The fix prevents re-rendering components once in renderedRoots. If users update props in DOM and call reactOnRailsPageLoaded() again, component will be skipped. Worth documenting if this is a known limitation.

Security Considerations

No security concerns identified.

Overall Assessment

Verdict: Excellent fix, ready to merge after addressing the changelog.

Risk Level: Low - minimal code change, comprehensive tests, no breaking changes.

Recommendation: Add changelog entry, confirm E2E tests pass, then merge with confidence.

Great work on this fix!

Code Review - PR #2211

Thank you for the fix! This addresses a real bug in the beta version. Here's my comprehensive review:

✅ Strengths

1. Excellent Root Cause Analysis: The fix correctly identifies that renderedRoots already tracks rendered components, and the check !!domNode.innerHTML was incorrectly treating client-rendered content as server-rendered.

2. Minimal, Surgical Change: The fix adds just 8 lines of code at the right place (lines 84-89 in ClientRenderer.ts), preventing re-rendering without changing the overall logic flow.

3. Comprehensive Test Coverage:

   • Unit test validates the fix at the function level
   • E2E Playwright tests cover realistic user scenarios
   • Manual test page with visual feedback for QA

4. Good Documentation: Clear comments explain WHY the check exists, not just WHAT it does.

🔍 Code Quality Observations

ClientRenderer.ts Changes

Line 84-89: The guard clause is well-placed and properly returns early.

if (renderedRoots.has(domNodeId)) {
  if (trace) {
    console.log(`Skipping already rendered component: ${name} (dom id: ${domNodeId})`);
  }
  return;
}

✅ Good:

• Consistent with existing trace logging pattern
• Early return prevents unnecessary work
• Log message is descriptive

Test Quality

ClientRenderer.test.ts (lines 206-263):

⚠️ Minor Concern: The test mocks reactHydrateOrRender but doesn't verify the mock wasn't called - it only checks the call count didn't increase. This is correct but could be more explicit:

// Current approach (works but indirect)
expect(mockHydrateOrRender.mock.calls.length).toBe(callCountAfterFirstRender);

// More explicit alternative
expect(mockHydrateOrRender).not.toHaveBeenCalledWith(
  expect.anything(),
  expect.anything(),
  expect.anything()
);

Verdict: Current approach is fine, but the test name could be even clearer about what it's testing.

E2E Tests (page_loaded_multiple_calls.spec.js)

✅ Excellent:

• Tests cover single addition, multiple additions, and UI feedback
• Console error monitoring correctly filters for hydration-specific errors
• Realistic simulation of the async loading scenario from issue Hydration mismatch with React 19 and the latest beta version of this gem #2210

📝 Documentation Suggestion: Consider adding a comment in the spec explaining why this test is critical for regression prevention.

🐛 Potential Issues

1. Edge Case: Manually Cleared DOM Nodes

Scenario: What if user code manually clears a DOM node's innerHTML after it's been rendered?

// Component rendered
ReactOnRails.reactOnRailsPageLoaded();

// User code clears the DOM
document.getElementById('MyComponent-1').innerHTML = '';

// Second call - will skip rendering due to renderedRoots.has() check
ReactOnRails.reactOnRailsPageLoaded();

Impact: The component won't re-render because it's still in renderedRoots.

Assessment: This is an edge case and probably acceptable behavior (user shouldn't manually manipulate React-managed DOM). But worth documenting.

Recommendation: Add a note in the JSDoc comment for renderElement or reactOnRailsPageLoaded about this assumption.

2. Memory Management for SPA-style Navigation

The current renderedRoots Map grows indefinitely. For apps with lots of dynamic components (e.g., Turbo/Hotwire navigation), this could theoretically accumulate entries.

Current cleanup: unmountAllComponents() is called on onPageUnloaded, which should handle full page navigations.

Question: Does Turbo/Hotwire trigger onPageUnloaded? If not, renderedRoots might accumulate across Turbo navigations.

Recommendation:

• Verify Turbo integration clears renderedRoots on Turbo page changes
• Consider adding a public ReactOnRails.clearRenderedRoots() API for manual cleanup if needed

3. Test Checkbox Not Marked Complete

In the PR description, this checkbox is unchecked:

- [ ] Playwright E2E test: async_page_loaded_test page should show no hydration errors

Action: Please run the Playwright test locally and mark it as complete, or document if there are blockers.

🔒 Security Considerations

✅ No security concerns identified. The change:

• Doesn't introduce XSS risks (no DOM manipulation)
• Doesn't expose internal state
• Doesn't change authentication/authorization logic

⚡ Performance Considerations

✅ Positive Impact: The early return on line 88 prevents unnecessary:

• Component registry lookups
• React element creation
• ReactDOM calls

The Map.has() check is O(1), so negligible overhead.

Estimated performance improvement: For apps calling reactOnRailsPageLoaded() multiple times with N components, this eliminates (calls - 1) × N unnecessary re-renders.

📋 Recommendations

High Priority

1. Mark Playwright test as complete or document why it's pending
2. Run RuboCop: Per CLAUDE.md, bundle exec rubocop is mandatory before commit
3. Verify Turbo/Hotwire integration: Ensure renderedRoots is properly cleared on Turbo page changes

Medium Priority

4. Add JSDoc comment to reactOnRailsPageLoaded() documenting the "already rendered" behavior:

   /**
    * Public API function that can be called to render components after they are loaded.
    * Safe to call multiple times - already-rendered components will be skipped.
    * 
    * @param domId - Optional specific component ID. If omitted, renders all components.
    * @note Components are tracked in memory and won't re-render on subsequent calls.
    *       If you need to force re-render, unmount the component first.
    */

5. Consider adding a test for the Turbo integration scenario if applicable

Low Priority

6. Changelog entry format: The PR description has good changelog content, but verify it follows the format in CLAUDE.md (with PR link format)

🎯 Final Verdict

LGTM with minor recommendations ✅

This is a solid fix for a real bug. The implementation is clean, well-tested, and should have minimal risk. The concerns raised are edge cases that likely won't affect most users, but documenting them would improve long-term maintainability.

Estimated CI Risk: Low - the change is defensive (adds a guard clause) and comprehensive tests should catch regressions.

📝 Pre-Merge Checklist (from CLAUDE.md)

Based on the project's requirements:

• bundle exec rubocop runs with zero offenses
• All files end with newline character
• Playwright tests pass locally (mark checkbox in PR description)
• CHANGELOG.md updated (verify format matches project standards)
• Consider adding to CHANGELOG.md if not already there

Note: Please verify the above before merging. The CLAUDE.md guidelines are very explicit about these requirements.

Great work on the fix! Let me know if you'd like me to elaborate on any of these points. 🚀

@AbanoubGhadban:

• Verified failure and the fix manually

Code Review: Fix hydration mismatch when reactOnRailsPageLoaded() called multiple times

✅ Overall Assessment

This is a well-implemented fix with comprehensive testing. The solution correctly addresses the root cause and includes both unit and E2E tests. Great work!

🎯 Strengths

1. Correct Root Cause Analysis: The fix properly identifies that the issue is re-rendering already-rendered components, not a hydration problem per se.

2. Robust Edge Case Handling: The code handles the DOM node replacement scenario (lines 94-113 in ClientRenderer.ts), which shows thoughtful consideration of real-world usage patterns.

3. Excellent Test Coverage:

   • Unit tests verify the core logic
   • E2E tests validate the actual user scenario
   • Tests cover both single and multiple dynamic additions

4. Clear Documentation: Inline comments explain the "why" behind the logic, which will help future maintainers.

🔍 Code Quality Observations

ClientRenderer.ts changes (lines 81-113)

Positive:

• The isConnected check is smart - it ensures we're checking the actual DOM state, not just object equality
• Proper cleanup of replaced nodes prevents memory leaks
• Error handling during unmount is appropriate (swallowing errors for replaced nodes)

Minor Concern:

const sameNode = existing.domNode === domNode && existing.domNode.isConnected;

This check compares existing.domNode === domNode AND existing.domNode.isConnected. However, if the nodes are the same object reference, we don't actually need to check .isConnected on the new domNode - we're only checking the existing one. This is fine, but could be slightly more explicit:

const sameNode = existing.domNode === domNode && domNode.isConnected;

Since they're the same reference, this is functionally identical, just slightly more semantically clear.

🧪 Testing

Unit Tests (ClientRenderer.test.ts):

• ✅ Tests the skip behavior correctly
• ✅ Tests the DOM replacement scenario
• ✅ Good use of mock call counting to verify behavior

E2E Tests (page_loaded_multiple_calls.spec.js):

• ✅ Comprehensive console error monitoring
• ✅ Tests multiple scenarios (2, 3, 4 components)
• ✅ Validates both visual rendering and absence of errors

Suggestion: Consider adding a test case for rapid successive calls (e.g., calling reactOnRailsPageLoaded() twice in quick succession before React finishes rendering). This could reveal race conditions if they exist.

📝 Documentation & Changelog

⚠️ MISSING: Changelog entry

According to the project's CLAUDE.md, user-visible bug fixes should be documented in /CHANGELOG.md. This fix should include an entry like:

#### Fixed
[PR 2211](https://github.com/shakacode/react_on_rails/pull/2211) by [justin808](https://github.com/justin808): Fixed hydration mismatch errors when `reactOnRailsPageLoaded()` is called multiple times for asynchronously loaded content. Previously rendered client-side components are now correctly skipped during subsequent calls.

🔒 Security Considerations

✅ No security concerns identified. The changes are purely internal logic improvements.

⚡ Performance Considerations

Positive:

• The renderedRoots Map lookup is O(1), so no performance degradation
• Early return for already-rendered components saves unnecessary work
• Proper cleanup prevents memory leaks

Minor optimization opportunity:
The isConnected check happens on every call. For pages with many components called frequently, you could consider a WeakMap-based approach, but the current implementation is perfectly fine for typical usage patterns.

🎨 Code Style

✅ Follows project conventions
✅ Proper TypeScript typing
✅ Clear variable names
✅ Appropriate use of early returns

🐛 Potential Edge Cases to Consider

1. What if domNode is removed from DOM but not replaced?

   • Current behavior: Will re-render into the cached (disconnected) node
   • This might be unexpected. Consider adding:

   if (existing && !existing.domNode.isConnected) {
     // Node was removed, clean up and re-render
     renderedRoots.delete(domNodeId);
   }

2. What if component props change but DOM node is the same?

   • Current behavior: Component won't re-render with new props
   • This might be intentional, but worth documenting if it's expected

📋 Action Items

Before merging:

1. ✅ Add CHANGELOG.md entry (as shown above)
2. ⚠️ Consider the disconnected node edge case
3. ⚠️ Consider documenting the props-change behavior

Optional enhancements:

• Add a test for rapid successive calls
• Consider adding a debug mode that logs when components are skipped

🎉 Conclusion

This is a high-quality fix that demonstrates good understanding of React rendering lifecycle and edge case handling. The comprehensive test coverage gives confidence that this won't regress. With the changelog entry added, this is ready to merge!

Recommendation: Approve with minor changelog addition

@claude review this PR

Claude finished @justin808's task in 2m 7s —— View job

PR Review for #2211: Fix hydration mismatch when reactOnRailsPageLoaded() called multiple times

✅ Overall Assessment

Verdict: LGTM with one minor action item (CHANGELOG entry)

This is an excellent fix for issue #2210. The implementation is surgical, well-tested, and handles edge cases thoughtfully. The root cause analysis is spot-on, and the solution is minimal yet comprehensive.

🎯 Strengths

1. Correct Root Cause Analysis

The PR correctly identifies that calling reactOnRailsPageLoaded() multiple times caused the shouldHydrate check (!!domNode.innerHTML) to incorrectly treat previously client-rendered content as server-rendered content. This is the exact issue.

2. Minimal, Surgical Implementation (ClientRenderer.ts:81-113)

• Only 32 lines of production code added
• Leverages existing renderedRoots Map infrastructure (line 15)
• Clean early return pattern prevents unnecessary work
• Handles both React 18+ Root API and legacy unmount gracefully

Key logic (lines 84-93):

const existing = renderedRoots.get(domNodeId);
if (existing) {
  const sameNode = existing.domNode === domNode && existing.domNode.isConnected;
  if (sameNode) {
    // Skip re-rendering same component
    return;
  }
  // DOM node was replaced - clean up old root and continue
}

3. Excellent Edge Case Handling (lines 94-113)

The fix intelligently handles the replaced DOM node scenario that CodeRabbit identified:

• Detects when DOM node with same ID was replaced (different object reference)
• Properly unmounts the old root to prevent memory leaks
• Allows re-rendering to the new DOM node
• Gracefully handles unmount errors (line 106-111) since old DOM is gone

4. Comprehensive Test Coverage

Unit Tests (ClientRenderer.test.ts:206-311):

• ✅ Tests skipping already-rendered components (lines 231-266)
• ✅ Tests re-rendering when DOM node is replaced (lines 268-310)
• ✅ Properly mocks reactHydrateOrRender and verifies call counts

E2E Tests (page_loaded_multiple_calls.spec.js):

• ✅ Tests single component addition (lines 50-104)
• ✅ Tests multiple additions (3 dynamic components, lines 106-151)
• ✅ Tests DOM node replacement scenario (lines 169-205)
• ✅ Tests multiple replacements (lines 207-228)
• ✅ Monitors console for hydration errors correctly

Test Page (async_page_loaded_test.html.erb):

• ✅ Excellent manual test page with clear instructions
• ✅ Visual feedback for both scenarios (add and replace)
• ✅ Captures and displays hydration errors in real-time
• ✅ Demonstrates the fix working correctly

5. Clear Documentation

• Inline comments explain the why, not just the what (lines 120-122)
• Test descriptions reference issue Hydration mismatch with React 19 and the latest beta version of this gem #2210
• Test page has clear scenario descriptions

🔍 Code Quality Observations

ClientRenderer.ts

Line 87: isConnected Check

const sameNode = existing.domNode === domNode && existing.domNode.isConnected;

Smart use of .isConnected to verify the DOM node is still in the document. This prevents skipping components whose nodes were removed but not replaced.

Lines 96-105: Dual Unmount Support

if (supportsRootApi && existing.root && typeof existing.root === 'object' && 'unmount' in existing.root) {
  existing.root.unmount();
} else {
  unmountComponentAtNode(existing.domNode);
}

Proper handling of both React 18+ Root API and legacy unmount. The type guards are appropriate.

Line 123: Hydration Check Now Correct

const shouldHydrate = !!domNode.innerHTML;

The comment above (lines 120-122) correctly explains why this now works: previously rendered components are skipped above, so innerHTML only indicates server-rendered content.

Unit Tests

Minor: Mock Clearing (line 258)

const mockHydrateOrRender = require('../src/reactHydrateOrRender.ts').default as jest.Mock;

Consider using jest.clearAllMocks() in beforeEach to ensure clean state between tests. Current approach works but could be more explicit.

Call Count Verification (line 265)

expect(mockHydrateOrRender.mock.calls.length).toBe(callCountAfterFirstRender);

This correctly verifies the mock wasn't called again. Could be slightly more explicit with:

expect(mockHydrateOrRender).not.toHaveBeenCalledTimes(callCountAfterFirstRender + 1);

But current approach is fine.

E2E Tests

Console Error Filtering (lines 59-66)

if (text.toLowerCase().includes('hydrat') || 
    text.toLowerCase().includes('did not match') || 
    text.toLowerCase().includes('mismatch'))

Good filtering for hydration-specific errors. This prevents false positives from other console errors.

Test Robustness
The tests wait for networkidle and use proper Playwright selectors. Well-structured.

⚠️ Required Action Item

Missing CHANGELOG.md Entry

Per CLAUDE.md guidelines:

Update /CHANGELOG.md for open-source features, bug fixes, breaking changes, deprecations, and performance improvements using format: [PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)

This is a user-visible bug fix that should be documented.

Suggested entry for the Unreleased section under #### Fixed:

#### Fixed
- Fixed hydration mismatch errors when `reactOnRailsPageLoaded()` is called multiple times for asynchronously loaded content. Previously rendered client-side components are now correctly skipped during subsequent calls, preventing React from attempting to hydrate already-rendered content. [PR 2211](https://github.com/shakacode/react_on_rails/pull/2211) by [justin808](https://github.com/justin808). Fixes [issue #2210](https://github.com/shakacode/react_on_rails/issues/2210).

🧪 Testing Verification

Based on the PR description and comments:

• ✅ Unit test passes (confirmed by CI)
• ✅ Manual testing completed (user verified with screenshots)
• ⚠️ E2E test status: The PR description shows the Playwright test checkbox unchecked, but CI shows it passing. Recommendation: Mark the checkbox as complete in the PR description.

🔒 Security & Performance

Security

✅ No security concerns identified. The changes:

• Don't introduce XSS risks
• Don't expose internal state
• Properly sanitize/validate inputs

Performance

✅ Positive performance impact:

• Map.has() and Map.get() are O(1) operations
• Early return prevents unnecessary React element creation and rendering
• For apps calling reactOnRailsPageLoaded() N times with M components, this eliminates (N-1) × M unnecessary re-renders
• Proper cleanup prevents memory leaks

💡 Potential Enhancements (Optional)

1. Extract Unmount Logic (Low Priority)

The unmount pattern is duplicated at lines 96-105 and 208-214. Consider extracting to a helper function:

function unmountRoot(entry: { root: RenderReturnType; domNode: Element }): void {
  if (supportsRootApi && entry.root && typeof entry.root === 'object' && 'unmount' in entry.root) {
    entry.root.unmount();
  } else {
    unmountComponentAtNode(entry.domNode);
  }
}

This is a nice-to-have, not blocking.

2. JSDoc Enhancement (Low Priority)

Consider adding a note to the reactOnRailsPageLoaded JSDoc about the skip behavior:

/**
 * Public API function that can be called to render components after they are loaded.
 * Safe to call multiple times - already-rendered components will be skipped.
 * 
 * @note Components are tracked and won't re-render on subsequent calls unless 
 *       the DOM node is replaced. If you need to force re-render with new props,
 *       use reactOnRailsComponentLoaded(domId) on individual components.
 */

📋 Pre-Merge Checklist

Per CLAUDE.md requirements:

• Code follows project conventions
• TypeScript typing is correct
• Tests are comprehensive (unit + E2E)
• Manual testing completed (verified by user)
• CHANGELOG.md updated ← Action required
• bundle exec rubocop passes (assumed from CI)
• Files end with newline (assumed from CI)
• Prettier formatting (assumed from CI)

🎉 Final Recommendation

LGTM - Approve after CHANGELOG entry is added

This is a high-quality fix that:

• ✅ Correctly diagnoses and fixes the root cause
• ✅ Has minimal code footprint (32 production lines)
• ✅ Includes comprehensive tests (unit + E2E + manual)
• ✅ Handles edge cases thoughtfully (DOM replacement)
• ✅ Has no breaking changes or security issues
• ✅ Improves performance

Risk Assessment: Low

• Changes are defensive (adds guards)
• Comprehensive test coverage reduces regression risk
• No API changes

Action Item: Add CHANGELOG.md entry, then merge with confidence!

Great work on this fix! 🚀

Code Review: Fix hydration mismatch when reactOnRailsPageLoaded() called multiple times

Overall Assessment

This PR effectively addresses issue #2210 with a well-thought-out solution. The fix is elegant, properly tested, and includes comprehensive E2E coverage.

Core Fix Analysis

The Problem: When reactOnRailsPageLoaded() is called multiple times, previously client-rendered components were being re-hydrated because !!domNode.innerHTML returned true, causing hydration mismatch errors in React 19.

The Solution: Track rendered roots and skip re-rendering components already mounted, while handling DOM node replacement edge cases.

Strengths

1. Correct DOM node identity check using existing.domNode === domNode && existing.domNode.isConnected
2. Properly handles DOM node replacement (same ID, different element)
3. Safe unmounting with try-catch for disconnected nodes
4. Comprehensive logging for debugging
5. Updated comments accurately explain the behavior

Recommendations

High Priority:

• Verify Playwright tests pass in CI
• Verify no memory leaks in SPAs - check if onPageUnloaded cleans up renderedRoots Map

Medium Priority:

• Consider documenting DOM replacement scenario more explicitly
• Add test for isConnected === false edge case

Low Priority:

• Document why unmount errors are silently ignored
• Consider improving type safety for unmount type guard

Test Coverage

Excellent test coverage with both unit tests and comprehensive E2E tests covering initial load, multiple calls, multiple additions, DOM replacement, and multiple replacements.

Security & Performance

No security concerns. Performance is good with O(1) Map lookups and early returns.

Verdict

Approve with minor recommendations

This is high-quality work that correctly fixes the root cause with comprehensive testing. Recommendations are mostly for documentation and edge case coverage, not blocking issues.

@claude review this PR.

Claude finished @justin808's task in 3m 6s —— View job

PR Review for #2211: Fix hydration mismatch when reactOnRailsPageLoaded() called multiple times

✅ Overall Assessment

Verdict: Excellent fix - LGTM with one required action (CHANGELOG entry)

This PR provides a clean, well-tested solution to issue #2210. The implementation is minimal yet comprehensive, with excellent test coverage and thoughtful handling of edge cases.

🎯 Core Fix Analysis

The Problem (Lines 81-92 in ClientRenderer.ts)

When reactOnRailsPageLoaded() is called multiple times (e.g., for dynamically loaded content), previously rendered client-side components were being re-hydrated because:

• The shouldHydrate check (!!domNode.innerHTML) returned true for already-rendered components
• React tried to hydrate content that wasn't server-rendered, causing hydration mismatch errors in React 19

The Solution

Brilliant use of existing infrastructure: The fix leverages the existing renderedRoots Map to track which components have been rendered, and adds smart logic to:

1. Skip re-rendering same components (lines 84-92):

   const existing = renderedRoots.get(domNodeId);
   if (existing) {
     const sameNode = existing.domNode === domNode && existing.domNode.isConnected;
     if (sameNode) {
       return; // Skip - component already rendered
     }

2. Handle DOM node replacement (lines 94-113):

   • Detects when DOM node with same ID was replaced (different object reference)
   • Properly unmounts the old root to prevent memory leaks
   • Allows re-rendering to the new DOM node
   • Handles both React 18+ Root API and legacy unmount

3. Fix hydration check (line 123):

   • Now shouldHydrate = !!domNode.innerHTML correctly identifies only server-rendered content
   • Previously rendered components are filtered out above, so this check is accurate

✨ Strengths

1. Minimal Code Footprint

Only 37 lines added to production code, with zero breaking changes. The fix is surgical and focused.

2. Smart Edge Case Handling

The CodeRabbit-identified edge case (DOM node replacement with same ID) is brilliantly handled:

• Uses existing.domNode === domNode to detect replacement
• Checks isConnected to verify node is still in document
• Properly cleans up old root before rendering new one
• Swallows unmount errors gracefully (old DOM is already gone)

3. Excellent Test Coverage

Unit Tests (ClientRenderer.test.ts:206-311):

• ✅ Tests skipping already-rendered components
• ✅ Tests re-rendering when DOM node is replaced
• ✅ Proper mock usage and call count verification

E2E Tests (page_loaded_multiple_calls.spec.js):

• ✅ Initial load without errors
• ✅ Single dynamic addition
• ✅ Multiple dynamic additions (3 components)
• ✅ DOM node replacement scenario
• ✅ Multiple DOM replacements
• ✅ Console error monitoring with proper filtering

Manual Test Page (manual_render_test.html.erb):

• ✅ Clear instructions for QA
• ✅ Visual feedback for both scenarios
• ✅ Real-time hydration error detection
• ✅ Tests both addition and replacement

4. Performance Benefits

• Map.has() and Map.get() are O(1) operations
• Early return prevents unnecessary React element creation
• For apps calling reactOnRailsPageLoaded() N times with M components, this eliminates (N-1) × M unnecessary re-renders
• Proper cleanup prevents memory leaks

5. Clean Architecture

• Leverages existing renderedRoots Map (line 15) - no new data structures
• Integrates with existing cleanup via onPageUnloaded (line 223)
• Follows existing patterns for React 18/legacy compatibility

🔍 Code Quality Observations

ClientRenderer.ts

Line 87: Smart isConnected Check

const sameNode = existing.domNode === domNode && existing.domNode.isConnected;

Excellent use of .isConnected to verify the DOM node is still in the document. This prevents skipping components whose nodes were removed but not replaced.

Lines 96-105: Dual Unmount Support

if (supportsRootApi && existing.root && typeof existing.root === 'object' && 'unmount' in existing.root) {
  existing.root.unmount();
} else {
  unmountComponentAtNode(existing.domNode);
}

Proper handling of both React 18+ Root API and legacy unmount. The type guards are appropriate.

Lines 106-111: Graceful Error Handling

catch (unmountError) {
  // Ignore unmount errors for replaced nodes
  if (trace) {
    console.log(`Error unmounting replaced component: ${name}`, unmountError);
  }
}

Smart to swallow unmount errors here - if the DOM node was replaced, unmounting the old node may legitimately fail, and we want to proceed with rendering to the new node.

Lines 120-122: Updated Comment
The comment correctly explains why the innerHTML check now works - it's the combination of the skip logic above that makes this accurate.

Unit Tests

Line 258: Mock Retrieval Pattern

const mockHydrateOrRender = require('../src/reactHydrateOrRender.ts').default as jest.Mock;

This works but could be slightly cleaner with jest.clearAllMocks() in beforeEach to ensure pristine state. Current approach is functional though.

Line 265: Call Count Verification

expect(mockHydrateOrRender.mock.calls.length).toBe(callCountAfterFirstRender);

This correctly verifies the mock wasn't called again. Could be slightly more explicit with toHaveBeenCalledTimes() but current approach is fine.

E2E Tests

Lines 62-68: Comprehensive Error Filtering

if (text.toLowerCase().includes('hydrat') || 
    text.toLowerCase().includes('did not match') || 
    text.toLowerCase().includes('mismatch'))

Excellent filtering for hydration-specific errors. This prevents false positives from other console errors.

Line 197: Hard-coded Timeout

await page.waitForTimeout(200);

This is a minor flake risk. Consider using page.waitForFunction() or checking for a specific DOM state instead. However, for a 200ms timeout this is likely acceptable.

⚠️ Required Action Items

🚨 HIGH PRIORITY: Missing CHANGELOG Entry

Per CLAUDE.md requirements:

Update /CHANGELOG.md for open-source features, bug fixes, breaking changes, deprecations, and performance improvements using format: [PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)

This is a user-visible bug fix that should be documented.

Suggested entry for the Unreleased section under #### Fixed:

#### Fixed
- Fixed hydration mismatch errors when `reactOnRailsPageLoaded()` is called multiple times for asynchronously loaded content. Previously rendered client-side components are now correctly skipped during subsequent calls, preventing React from attempting to hydrate already-rendered content. Includes proper handling of DOM node replacement scenarios (same ID, different element). [PR 2211](https://github.com/shakacode/react_on_rails/pull/2211) by [justin808](https://github.com/justin808). Fixes [#2210](https://github.com/shakacode/react_on_rails/issues/2210).

💡 Optional Enhancements (Low Priority)

1. Extract Unmount Logic (Refactoring Opportunity)

The unmount pattern is duplicated at lines 96-105 and 205-214. Consider extracting to a helper:

function unmountRoot(entry: { root: RenderReturnType; domNode: Element }): void {
  if (supportsRootApi && entry.root && typeof entry.root === 'object' && 'unmount' in entry.root) {
    entry.root.unmount();
  } else {
    unmountComponentAtNode(entry.domNode);
  }
}

This is a nice-to-have, not blocking.

2. JSDoc Enhancement

Consider adding a note to the reactOnRailsPageLoaded JSDoc about the skip behavior:

/**
 * Public API function that can be called to render components after they are loaded.
 * Safe to call multiple times - already-rendered components will be skipped.
 * 
 * @note Components are tracked and won't re-render on subsequent calls unless 
 *       the DOM node is replaced. To force re-render with new props, use
 *       reactOnRailsComponentLoaded(domId) on individual components.
 */

🔒 Security & Performance

Security

✅ No security concerns identified. The changes:

• Don't introduce XSS risks
• Don't expose internal state
• Properly sanitize/validate inputs

Performance

✅ Positive performance impact:

• Map.has() and Map.get() are O(1) operations - no overhead
• Early return prevents unnecessary React element creation and rendering
• Estimated improvement: For apps calling reactOnRailsPageLoaded() N times with M components, eliminates (N-1) × M unnecessary re-renders
• Proper cleanup prevents memory leaks

📋 Pre-Merge Checklist

Per CLAUDE.md requirements:

• Code follows project conventions
• TypeScript typing is correct
• Tests are comprehensive (unit + E2E + manual)
• Manual testing completed (verified by user with screenshots)
• CHANGELOG.md updated ← Action required
• bundle exec rubocop passes (assumed from CI)
• Files end with newline (assumed from CI)
• Prettier formatting (assumed from CI)

🎉 Final Recommendation

LGTM - Approve after CHANGELOG entry is added

This is high-quality work that:

• ✅ Correctly diagnoses and fixes the root cause
• ✅ Has minimal code footprint (37 production lines)
• ✅ Includes comprehensive tests (unit + E2E + manual)
• ✅ Handles edge cases thoughtfully (DOM replacement)
• ✅ Has no breaking changes or security issues
• ✅ Improves performance
• ✅ User has manually verified the fix

Risk Assessment: Low

• Changes are defensive (adds guards)
• Comprehensive test coverage reduces regression risk
• No API changes
• Backward compatible

Action Item: Add CHANGELOG.md entry, then merge with confidence!

Great work on this fix! 🚀

Additional Context

Comparison with CodeRabbit Reviews:
CodeRabbit's reviews correctly identified the DOM replacement edge case, which this PR handles excellently. The implementation addresses all their concerns:

• ✅ Guards against "same id, new DOM node" scenario
• ✅ Unmounts old roots properly
• ✅ No memory leaks
• ✅ Comprehensive test coverage for the edge case

User Verification:
The user manually tested the fix and provided screenshots showing:

• Before fix: Hydration mismatch errors in console
• After fix: No errors, components render correctly

This gives additional confidence beyond the automated tests.

Code Review - PR #2211

Overall Assessment: ✅ LGTM with minor suggestions

This is a well-thought-out fix for Issue #2210. The implementation correctly addresses the hydration mismatch problem while also handling the edge case of replaced DOM nodes.

✅ Strengths

1. Correct Root Cause Analysis

The fix properly identifies that reactOnRailsPageLoaded() was re-hydrating already-rendered components because !!domNode.innerHTML was true for client-rendered content. The solution of tracking rendered roots prevents this.

2. Handles Edge Case Well

The CodeRabbit feedback about replaced DOM nodes (same ID, new element) is correctly handled:

• Checks if it's the exact same DOM node instance via reference equality
• Checks if the node is still connected to the document (isConnected)
• Properly unmounts old root before rendering to the new node
• This prevents memory leaks from orphaned React roots

3. Excellent Test Coverage

• Unit tests verify the core logic (skip already-rendered, render replaced nodes)
• E2E tests cover multiple scenarios comprehensively
• Manual test page provides easy verification during development
• Tests are well-documented with clear intent

4. Good Code Quality

• Clear comments explaining the logic
• Defensive error handling (try/catch for unmount)
• Trace logging for debugging
• Follows existing code patterns

🔍 Security & Performance Considerations

Security: ✅ No Concerns

• No XSS risks introduced
• Proper JSON parsing with type safety
• No new external dependencies
• Standard DOM API usage

Performance: ✅ Good

• renderedRoots.get() is O(1) - minimal overhead
• Only runs when reactOnRailsPageLoaded() is called (not on every render)
• Proper cleanup prevents memory leaks
• Early return for already-rendered components avoids unnecessary work

💡 Suggestions for Improvement

1. Minor: Type Safety in Unmount Check (Low Priority)

Current code (lines 96-100):

if (
  supportsRootApi &&
  existing.root &&
  typeof existing.root === 'object' &&
  'unmount' in existing.root
) {

Suggestion: The type checking is a bit verbose. Consider using a type guard:

function isReactRoot(root: RenderReturnType): root is ReactRoot {
  return supportsRootApi && root !== null && typeof root === 'object' && 'unmount' in root;
}

// Then use:
if (isReactRoot(existing.root)) {
  existing.root.unmount();
} else {
  unmountComponentAtNode(existing.domNode);
}

Impact: Minor readability improvement, not critical.

2. Minor: Consider Logging Cleanup (Low Priority)

Current code: Only logs errors during unmount, not successful cleanup.

Suggestion: Add trace logging for the cleanup path to help debugging:

if (trace) {
  console.log(`Cleaning up replaced component: \${name} (dom id: \${domNodeId})`);
}

Impact: Better observability during development.

3. Consider: Edge Case Documentation (Documentation)

The logic handles an important edge case (replaced nodes), but it might not be obvious to future maintainers why we check isConnected.

Suggestion: Add a comment explaining the edge case:

// Only skip if it's the exact same DOM node and it's still connected to the document
// If the DOM node was replaced (e.g., via innerHTML or async HTML injection), 
// we need to unmount the old root and render to the new node to prevent:
// 1. Memory leaks from orphaned React roots
// 2. Components not rendering when legitimately needed
const sameNode = existing.domNode === domNode && existing.domNode.isConnected;

🎯 Specific Code Review Notes

ClientRenderer.ts:87-93 ✅

The reference equality check (existing.domNode === domNode) is correct—it checks object identity, not just ID matching.

ClientRenderer.ts:106-111 ✅

Swallowing unmount errors for replaced nodes is appropriate—the old node is likely already detached and unable to unmount cleanly.

ClientRenderer.ts:141 ✅

Storing both root and domNode in the Map is necessary for the replacement check—good design.

Test Coverage ✅

The test suite covers:

• ✅ Skip already-rendered components
• ✅ Render to replaced DOM nodes
• ✅ Multiple consecutive additions
• ✅ Multiple consecutive replacements
• ✅ No hydration errors in any scenario

📋 Testing Recommendations

Before merge, verify:

1. Manual testing: Visit /manual_render_test and:

   • Click "Add Component" 3-5 times → Should see no console errors
   • Click "Replace Component" 3-5 times → Should see success messages
   • Check browser console for any hydration warnings

2. Regression testing: Ensure existing SSR components still hydrate correctly

3. CI status: All checks passing (currently in progress)

🚀 Conclusion

This is a high-quality fix that:

• ✅ Solves the reported issue correctly
• ✅ Handles edge cases properly
• ✅ Has comprehensive test coverage
• ✅ Maintains backward compatibility
• ✅ No security or performance concerns

The suggestions above are minor improvements, not blockers. The PR is ready to merge as-is.

Recommendation: Approve & Merge ✅

Reviewed using CLAUDE.md guidelines for code quality, testing, and best practices.

Code Review: Fix Hydration Mismatch on Multiple reactOnRailsPageLoaded() Calls

Overall Assessment: This is a well-implemented fix with excellent test coverage. The solution correctly addresses the root cause and handles edge cases thoughtfully.

Strengths

1. Root Cause Analysis

The fix correctly identifies that the issue stems from shouldHydrate = !!domNode.innerHTML returning true for previously client-rendered components. The solution (checking renderedRoots Map) is clean and efficient.

2. Excellent Test Coverage

• Unit tests: Cover both the skip scenario and the DOM replacement edge case
• E2E tests: Comprehensive Playwright tests with multiple scenarios including initial render, multiple dynamic additions, DOM node replacement, and console error detection
• Manual test page: Provides visual verification at /manual_render_test

3. Edge Case Handling

The DOM node replacement logic is particularly well done - existing.domNode === domNode && existing.domNode.isConnected correctly distinguishes between same render calls (skip) vs replaced DOM nodes (clean up old root, render to new node).

4. Cleanup on Replacement

Proper unmounting of the old root when a DOM node is replaced prevents memory leaks, with support for both React 18+ (createRoot API) and React 17- (render API).

Observations & Minor Suggestions

1. TypeScript Safety in Unmount Check

The unmount check is defensive but could benefit from a comment explaining it handles both React 18+ and React 17- gracefully.

2. Performance Consideration

The renderedRoots Map grows with every component render. In long-running SPAs, this could accumulate entries for components no longer on the page.

Minor concern: If someone programmatically removes components without calling React unmount, entries will leak. However, this is an edge case and the current implementation is fine for typical usage.

Future enhancement: Consider cleanup on page unload or provide a manual cleanup API for SPAs with dynamic content.

3. Test Quality Note

The unit tests verify mock call counts rather than actual DOM state changes. While this works, the E2E tests compensate by testing actual browser behavior, so overall coverage is excellent.

Security & Performance

• Security: No concerns - no user input handling or external data processing
• Performance: Minimal overhead (just a Map lookup). Skip logic actually improves performance by preventing unnecessary re-renders
• Memory: Minor consideration about Map growth (see above), but not a blocker

Test Coverage: Excellent

Unit Tests: Skip already-rendered components, Re-render when DOM node replaced

E2E Tests: Initial render, Multiple reactOnRailsPageLoaded() calls, Multiple dynamic additions, DOM replacement, Console error detection

Potential Issues: None Found

The implementation correctly handles:

1. Preventing duplicate renders of the same component
2. Allowing re-renders when DOM nodes are replaced
3. Cleaning up old roots to prevent memory leaks
4. Supporting both React 17- and React 18+ APIs

Recommendation: APPROVE

This PR is ready to merge pending CI passing.

Minor follow-ups (optional, can be done in future PRs):

1. Add comment explaining React 17/18 compatibility in unmount check
2. Consider explicit cleanup for long-running SPAs if Map growth becomes an issue
3. Document memory characteristics of renderedRoots Map

Great work on the thorough investigation and implementation!