Only initiate hydration after all subresources loaded #1988
Conversation
WalkthroughIntroduces an internal onPageReady(callback) wrapper that centralizes readiness handling via document.readyState and the "readystatechange" event; replaces prior initialization path to drive setupPageNavigationListeners. Tests updated to reflect readystatechange semantics. System specs removing pro/immediate-hydration contexts and two dependent tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as onPageLoaded/onPageUnloaded
participant OnReady as onPageReady(callback)
participant Document as document
participant Setup as setupPageNavigationListeners
Caller->>OnReady: request setup
OnReady->>Document: read `readyState`
alt readyState == "complete"
OnReady->>Setup: invoke callback immediately
else readyState != "complete"
OnReady->>Document: addEventListener("readystatechange", handler)
Document->>OnReady: "readystatechange" fires
OnReady->>OnReady: re-check `readyState`
OnReady->>Setup: invoke callback
OnReady->>Document: removeEventListener("readystatechange", handler)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-on-rails/src/pageLifecycle.ts (1)
72-78: Remove duplicate initialization of page navigation listeners.Both
onPageLoadedandonPageUnloadedcallonPageReady(setupPageNavigationListeners), which causes event listeners to be registered twice during initialization. Since these functions are called together increateReactOnRailsPro.ts(line 77-78) andCallbackRegistry.ts(line 63-70),setupPageNavigationListenersexecutes twice, adding duplicate event listeners for Turbo/Turbolinks navigation events. This causes callbacks to fire multiple times unintentionally.Only
onPageLoadedshould initialize navigation listeners. Remove the call toonPageReady(setupPageNavigationListeners)fromonPageUnloaded(line 85).
🧹 Nitpick comments (1)
packages/react-on-rails/src/pageLifecycle.ts (1)
61-70: Simplify the readystatechange listener pattern.The recursive pattern works but is unnecessarily complex. The listener recursively calls
onPageReady, which may register a new listener before the current one is removed. A clearer approach would check the readyState directly in the listener without recursion.Apply this diff to simplify:
-function onPageReady(callback: () => void) { - if (document.readyState === 'complete') { - callback(); - } else { - document.addEventListener('readystatechange', function onReadyStateChange() { - onPageReady(callback); - document.removeEventListener('readystatechange', onReadyStateChange); - }); - } -} +function onPageReady(callback: () => void) { + if (document.readyState === 'complete') { + callback(); + } else { + const listener = () => { + if (document.readyState === 'complete') { + document.removeEventListener('readystatechange', listener); + callback(); + } + }; + document.addEventListener('readystatechange', listener); + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-on-rails/src/pageLifecycle.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
packages/react-on-rails/src/pageLifecycle.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (1)
packages/react-on-rails/src/pageLifecycle.ts (1)
61-70: Approach aligns with PR objectives.The
onPageReadyfunction correctly waits fordocument.readyState === 'complete', ensuring all subresources (images, stylesheets, scripts) are fully loaded before initializing page navigation listeners and running callbacks. This aligns with the PR objective to prevent race conditions by deferring hydration until the page is fully ready.Note: The duplicate initialization issue flagged in the other comment must be resolved to fully achieve the PR's goal.
Code Review FeedbackSummaryThis PR addresses a critical race condition where components may attempt to hydrate before all subresources (including component registrations) are loaded. The change moves from waiting for ✅ Positives
|
Code Review: Only initiate hydration after all subresources loadedThank you for this PR @Judahmeek! I've reviewed the changes and here's my feedback: SummaryThis PR changes the page lifecycle initialization to wait for ✅ Strengths
🔍 Issues & Concerns1. Critical Logic Bug in
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-on-rails/tests/pageLifecycle.test.js (1)
84-108: Tests don't verify that callbacks execute after readystatechange fires.The tests at lines 84-108 verify that a
readystatechangelistener is added whenreadyStateis not'complete', but they never simulate the event firing to confirm the callback is eventually executed. This is why the critical bug inonPageReady(where the recursive call hits an early return) wasn't caught by the test suite.Add test coverage to simulate the readystatechange event and verify callback execution:
it('should execute callback when readystatechange fires', () => { setReadyState('loading'); const { onPageLoaded } = importPageLifecycle(); const callback = jest.fn(); onPageLoaded(callback); // Callback should not be called yet expect(callback).not.toHaveBeenCalled(); // Simulate document becoming ready setReadyState('complete'); const readyStateChangeHandler = addEventListenerSpy.mock.calls.find( call => call[0] === 'readystatechange' )[1]; readyStateChangeHandler(); // Now callback should have been executed expect(callback).toHaveBeenCalledTimes(1); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-on-rails/src/pageLifecycle.ts(2 hunks)packages/react-on-rails/tests/pageLifecycle.test.js(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
packages/react-on-rails/tests/pageLifecycle.test.jspackages/react-on-rails/src/pageLifecycle.ts
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
packages/react-on-rails/src/pageLifecycle.ts
🧬 Code graph analysis (1)
packages/react-on-rails/tests/pageLifecycle.test.js (1)
packages/react-on-rails/src/pageLifecycle.ts (1)
onPageLoaded(80-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: claude-review
Code Review for PR #1988SummaryThis PR changes the page lifecycle initialization to wait for ✅ Positive Aspects
🔴 Critical Issues1. Breaking Behavioral ChangeLocation: // BEFORE: Runs on 'interactive' OR 'complete'
if (document.readyState \!== 'loading') {
// AFTER: Only runs on 'complete'
if (document.readyState === 'complete') {Problem: This significantly delays hydration. The Impact: Users will see a longer delay before React components become interactive, especially on slow networks where images/fonts take time to load. Recommendation: Consider a more nuanced approach:
2. Incorrect Event Listener ChoiceLocation: document.addEventListener('readystatechange', function onReadyStateChange() {
onPageReady(callback);
document.removeEventListener('readystatechange', onReadyStateChange);
});Problem: Using Issues:
Better approach (if waiting for 'complete' is truly necessary): if (document.readyState === 'complete') {
callback();
} else {
document.addEventListener('load', callback, { once: true });
}Or using document.addEventListener('readystatechange', function onReadyStateChange() {
if (document.readyState === 'complete') {
callback();
document.removeEventListener('readystatechange', onReadyStateChange);
}
});3. Potential Infinite Recursion RiskLocation: While the Current flow:
This is unnecessarily convoluted.
|
Judahmeek
force-pushed
the
judahmeek/debug-image-example
branch
from
90971d0 to
c63c292
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-on-rails/tests/pageLifecycle.test.js (1)
49-49: Add spy for window.addEventListener to catch the correct event target.After fixing the production code to use
window.addEventListener('load', ...), the test spy on line 49 won't capture it since it only spies ondocument.addEventListener.Apply this diff to spy on both targets:
- addEventListenerSpy = jest.spyOn(document, 'addEventListener').mockImplementation(() => {}); - removeEventListenerSpy = jest.spyOn(document, 'removeEventListener').mockImplementation(() => {}); + const documentAddEventListenerSpy = jest.spyOn(document, 'addEventListener').mockImplementation(() => {}); + const documentRemoveEventListenerSpy = jest.spyOn(document, 'removeEventListener').mockImplementation(() => {}); + const windowAddEventListenerSpy = jest.spyOn(window, 'addEventListener').mockImplementation(() => {}); + const windowRemoveEventListenerSpy = jest.spyOn(window, 'removeEventListener').mockImplementation(() => {}); + + addEventListenerSpy = { + document: documentAddEventListenerSpy, + window: windowAddEventListenerSpy, + }; + removeEventListenerSpy = { + document: documentRemoveEventListenerSpy, + window: windowRemoveEventListenerSpy, + };Then update line 67-68 to restore all spies, and update assertions throughout the test file to check the appropriate target (e.g.,
addEventListenerSpy.windowfor 'load',addEventListenerSpy.documentfor turbo/turbolinks events).
♻️ Duplicate comments (1)
packages/react-on-rails/tests/pageLifecycle.test.js (1)
84-85: Test description doesn't match the readyState being tested.The description says
"interactive"but line 85 setsreadyStateto"loading". Since line 97 already tests the "loading" state, update line 85 to test "interactive" to ensure both non-complete states are covered.Apply this diff:
it('should wait for readystatechange when document.readyState is "interactive"', () => { - setReadyState('loading'); + setReadyState('interactive');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-on-rails/src/pageLifecycle.ts(1 hunks)packages/react-on-rails/tests/pageLifecycle.test.js(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
packages/react-on-rails/tests/pageLifecycle.test.jspackages/react-on-rails/src/pageLifecycle.ts
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
packages/react-on-rails/tests/pageLifecycle.test.js
🧬 Code graph analysis (1)
packages/react-on-rails/tests/pageLifecycle.test.js (1)
packages/react-on-rails/src/pageLifecycle.ts (1)
onPageLoaded(77-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
| expect(callback).toHaveBeenCalledTimes(1); | ||
| // Should not add DOMContentLoaded listener since readyState is not 'loading' | ||
| expect(addEventListenerSpy).not.toHaveBeenCalledWith('DOMContentLoaded', expect.any(Function)); | ||
| expect(addEventListenerSpy).not.toHaveBeenCalledWith('readystatechange', expect.any(Function)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Update outdated terminology to reflect 'load' event usage.
The test descriptions and comments reference "readystatechange" but the implementation now uses the 'load' event. Update the terminology for clarity:
- Line 81: "readystatechange" → "load event"
- Line 84: "wait for readystatechange" → "wait for load event"
- Lines 91-94: The comments and expectations are about the 'load' event, not readystatechange
- Line 97: "wait for readystatechange" → "wait for load event"
- Lines 106-107: Similar updates
After fixing the production bug to use window.addEventListener, also update line 94 and 107 expectations:
- expect(addEventListenerSpy).toHaveBeenCalledWith('load', expect.any(Function));
+ expect(addEventListenerSpy.window).toHaveBeenCalledWith('load', expect.any(Function));Also applies to: 84-84, 91-94, 97-97, 106-107
Code ReviewSummaryThis PR addresses a critical race condition in component hydration by ensuring that page lifecycle initialization only occurs after all subresources (images, stylesheets, scripts) are fully loaded. The change moves from ✅ Strengths1. Solves a Real Problem
2. Clean Implementation
3. Test Coverage
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/react-on-rails/src/pageLifecycle.ts (1)
62-78: CRITICAL: Callback never executes when document is not ready.The recursive call to
onPageReady(callback)at line 74 will always hit the early return at lines 65-67 becauseisPageLifecycleInitializedwas already set totrueat line 68. This meanscallback()will never execute when the page is still loading, breaking hydration entirely.Apply this diff to fix the logic:
function onPageReady(callback: () => void) { if (typeof window === 'undefined') return; if (isPageLifecycleInitialized) { return; } isPageLifecycleInitialized = true; if (document.readyState === 'complete') { callback(); } else { - document.addEventListener('readystatechange', function onReadyStateChange() { - onPageReady(callback); - document.removeEventListener('readystatechange', onReadyStateChange); - }); + document.addEventListener('readystatechange', function onReadyStateChange() { + if (document.readyState === 'complete') { + document.removeEventListener('readystatechange', onReadyStateChange); + callback(); + } + }); } }packages/react-on-rails/tests/pageLifecycle.test.js (1)
84-95: Test description doesn't match the readyState being tested.The test description says
"interactive"but line 85 setsreadyStateto"loading". Update the description to match the actual test behavior.Apply this diff to fix the description:
- it('should wait for readystatechange when document.readyState is "interactive"', () => { + it('should wait for readystatechange when document.readyState is "loading"', () => { setReadyState('loading');
🧹 Nitpick comments (1)
packages/react-on-rails/tests/pageLifecycle.test.js (1)
97-108: Consider testing the complete readystatechange flow.This test verifies that the listener is registered but doesn't test that the callback actually executes when
readystatechangefires andreadyStatebecomes'complete'. Consider adding a test that simulates the state change to ensure end-to-end behavior.Example test to add:
it('should execute callback when readystatechange fires with complete state', () => { setReadyState('loading'); const { onPageLoaded } = importPageLifecycle(); const callback = jest.fn(); onPageLoaded(callback); expect(callback).not.toHaveBeenCalled(); // Simulate readystatechange event setReadyState('complete'); const listener = addEventListenerSpy.mock.calls.find( call => call[0] === 'readystatechange' )?.[1]; listener?.(); expect(callback).toHaveBeenCalledTimes(1); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-on-rails/src/pageLifecycle.ts(2 hunks)packages/react-on-rails/tests/pageLifecycle.test.js(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
packages/react-on-rails/src/pageLifecycle.ts
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
packages/react-on-rails/src/pageLifecycle.ts
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
packages/react-on-rails/src/pageLifecycle.ts
🧬 Code graph analysis (1)
packages/react-on-rails/tests/pageLifecycle.test.js (1)
packages/react-on-rails/src/pageLifecycle.ts (1)
onPageLoaded(80-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: claude-review
🔇 Additional comments (1)
packages/react-on-rails/tests/pageLifecycle.test.js (1)
71-82: LGTM!Test correctly validates that no
readystatechangelistener is added whendocument.readyStateis already'complete'.
Code ReviewThank you for this PR! I've reviewed the changes to address the component registration race condition. Here's my detailed feedback: OverviewThis PR changes the page lifecycle initialization from waiting for 🔴 Critical Issues1. Potential Infinite Recursion in
|
Code Review for PR #1988Thank you for addressing the component registration race condition! I've reviewed the changes and have both positive feedback and concerns to share. ✅ Strengths
🚨 Critical Issues1. Logic Bug: Infinite Recursion Risk
|
Code ReviewOverviewThis PR changes the page lifecycle initialization from waiting for ✅ Strengths1. Addresses Real Race ConditionThe change correctly solves the component registration race condition by ensuring all scripts/resources are loaded before hydration begins. This aligns with the original behavior from PR #1540. 2. Clean Implementation
3. Test Updates Are AccurateThe test changes correctly reflect the new behavior:
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-on-rails/tests/pageLifecycle.test.js (1)
84-108: Duplicate test cases testing identical scenarios.Both test cases (lines 84-95 and 97-108) set
readyStateto'loading'and verify the exact same behavior. After fixing the description mismatch in line 84, these tests become completely identical.Either remove the duplicate test or modify one to test the
"interactive"state:it('should wait for readystatechange when document.readyState is "interactive"', () => { - setReadyState('loading'); + setReadyState('interactive'); const callback = jest.fn(); const { onPageLoaded } = importPageLifecycle(); onPageLoaded(callback); - // Should not call callback immediately since readyState is 'loading' + // Should not call callback immediately since readyState is 'interactive' expect(callback).not.toHaveBeenCalled(); - // Verify that a DOMContentLoaded listener was added when readyState is 'loading' + // Verify that a readystatechange listener was added when readyState is 'interactive' expect(addEventListenerSpy).toHaveBeenCalledWith('readystatechange', expect.any(Function)); }); - - it('should wait for readystatechange when document.readyState is "loading"', () => { - setReadyState('loading'); - const callback = jest.fn(); - const { onPageLoaded } = importPageLifecycle(); - - onPageLoaded(callback); - - // Should not call callback immediately since readyState is 'loading' - expect(callback).not.toHaveBeenCalled(); - // Verify that a DOMContentLoaded listener was added when readyState is 'loading' - expect(addEventListenerSpy).toHaveBeenCalledWith('readystatechange', expect.any(Function)); - });
♻️ Duplicate comments (1)
packages/react-on-rails/tests/pageLifecycle.test.js (1)
84-95: Test description doesn't match implementation.The test description on line 84 says
"interactive"but line 85 setsreadyStateto'loading'. This creates confusion about what state is actually being tested.Apply this diff to fix the description:
- it('should wait for readystatechange when document.readyState is "interactive"', () => { + it('should wait for readystatechange when document.readyState is "loading"', () => { setReadyState('loading');
🧹 Nitpick comments (1)
packages/react-on-rails/tests/pageLifecycle.test.js (1)
80-80: Update outdated comments referencing DOMContentLoaded.The comments reference
DOMContentLoadedbut the implementation now uses thereadystatechangeevent. Update the comments to reflect the current implementation.Apply this diff:
- // Should not add DOMContentLoaded listener since readyState is not 'loading' + // Should not add readystatechange listener since readyState is 'complete' expect(addEventListenerSpy).not.toHaveBeenCalledWith('readystatechange', expect.any(Function));- // Verify that a DOMContentLoaded listener was added when readyState is 'loading' + // Verify that a readystatechange listener was added when readyState is 'loading' expect(addEventListenerSpy).toHaveBeenCalledWith('readystatechange', expect.any(Function));- // Verify that a DOMContentLoaded listener was added when readyState is 'loading' + // Verify that a readystatechange listener was added when readyState is 'loading' expect(addEventListenerSpy).toHaveBeenCalledWith('readystatechange', expect.any(Function));Also applies to: 93-94, 106-107
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/react-on-rails/src/pageLifecycle.ts(2 hunks)packages/react-on-rails/tests/pageLifecycle.test.js(2 hunks)spec/dummy/spec/system/integration_spec.rb(0 hunks)
💤 Files with no reviewable changes (1)
- spec/dummy/spec/system/integration_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-on-rails/src/pageLifecycle.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
packages/react-on-rails/tests/pageLifecycle.test.js
🧬 Code graph analysis (1)
packages/react-on-rails/tests/pageLifecycle.test.js (1)
packages/react-on-rails/src/pageLifecycle.ts (1)
onPageLoaded(79-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
- GitHub Check: build
- GitHub Check: claude-review
|
I disagree with Claude & CodeRabbit regarding the recursion. I don't care how many event listeners are created. I only care that they don't actually do anything once |
Code ReviewThanks for working on this fix! I've reviewed the changes and here's my feedback: ✅ Strengths
🔍 Issues & ConcernsCritical: Potential infinite recursion bugIn document.addEventListener('readystatechange', function onReadyStateChange() {
document.removeEventListener('readystatechange', onReadyStateChange);
onPageReady(callback); // ⚠️ Recursive call
});Problem: If Suggested fix: Check if document.addEventListener('readystatechange', function onReadyStateChange() {
if (document.readyState === 'complete') {
document.removeEventListener('readystatechange', onReadyStateChange);
if (!isPageLifecycleInitialized) {
isPageLifecycleInitialized = true;
callback();
}
}
});Behavioral regression: duplicate initialization guard removedThe deleted test Issue: If Suggested fix: Move the initialization guard outside the readyState check: let isPageLifecycleInitialized = false;
function onPageReady(callback: () => void) {
if (typeof window === 'undefined') return;
if (isPageLifecycleInitialized) return; // Guard at top level
if (document.readyState === 'complete') {
isPageLifecycleInitialized = true;
callback();
} else {
document.addEventListener('readystatechange', function onReadyStateChange() {
if (document.readyState === 'complete') {
document.removeEventListener('readystatechange', onReadyStateChange);
if (!isPageLifecycleInitialized) {
isPageLifecycleInitialized = true;
callback();
}
}
});
}
}Test coverage gapThe removed test for duplicate initialization should be replaced with a test that verifies the new behavior doesn't add multiple listeners when called repeatedly while 📊 Performance ConsiderationsPositive: Waiting for Suggestion: Consider adding a comment explaining why 🔒 Security ConcernsNo security issues identified. 📝 Code Quality
✅ Recommendations
📋 Style & ConventionsCode follows the project's TypeScript conventions. Remember to run Overall, this PR addresses the right problem with a sensible solution, but needs refinement in the implementation to avoid edge cases and maintain proper guards against duplicate initialization. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/react-on-rails/tests/pageLifecycle.test.js (2)
80-81: Clarify the comment to reflect the actual readyState.The comment states "since readyState is not 'loading'", which is imprecise. The test sets readyState to 'complete' (line 72), so the comment should clearly reflect that the listener is not added because the page is already in the 'complete' state.
Apply this diff to make the comment more precise:
- // Should not add readystatechange listener since readyState is not 'loading' + // Should not add readystatechange listener since readyState is already 'complete' expect(addEventListenerSpy).not.toHaveBeenCalledWith('readystatechange', expect.any(Function));
91-91: Fix incorrect comment about readyState value.The comment states "since readyState is 'loading'", but line 85 sets the readyState to 'interactive', not 'loading'. This mismatch could confuse future readers.
Apply this diff to correct the comment:
- // Should not call callback immediately since readyState is 'loading' + // Should not call callback immediately since readyState is 'interactive' expect(callback).not.toHaveBeenCalled();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-on-rails/tests/pageLifecycle.test.js(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
packages/react-on-rails/tests/pageLifecycle.test.js
🧬 Code graph analysis (1)
packages/react-on-rails/tests/pageLifecycle.test.js (1)
packages/react-on-rails/src/pageLifecycle.ts (1)
onPageLoaded(79-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: claude-review
🔇 Additional comments (2)
packages/react-on-rails/tests/pageLifecycle.test.js (2)
97-108: LGTM!This test correctly verifies the behavior when readyState is 'loading'. The test description, readyState value, and comments are all consistent and accurate.
110-234: LGTM!The remaining test cases comprehensively cover different navigation libraries (Turbo, Turbolinks 5, Turbolinks 2), multiple callback handling, and server-side rendering scenarios. The test structure is well-organized and the expectations are clear and appropriate.
Code Review for PR #1988Thank you for working on this PR to address the component registration race condition. I've reviewed the changes and have the following feedback: ✅ Strengths
🔍 Issues & Concerns1. Critical Logic Error in
|
Code Review: Only initiate hydration after all subresources loadedOverviewThis PR reverts the hydration behavior to wait for Code Quality: ✅ GoodStrengths:
Areas for Improvement:1. Potential Infinite Recursion Risk
|
Code Review - PR #1988: Only initiate hydration after all subresources loadedOverviewThis PR addresses a critical race condition by ensuring React component hydration only occurs after the document is fully loaded ( ✅ Strengths
|
Code Review for PR #1988: Only initiate hydration after all subresources loadedSummaryThis PR reverts from Code Quality & Best Practices✅ Strengths
Potential Bugs & Issues🔴 Critical: Logic Error in
|
Code Review: Only initiate hydration after all subresources loadedOverviewThis PR addresses a critical race condition issue where component registration could fail due to async hydration being moved to Pro-only. The solution reverts to waiting for ✅ Strengths
🔍 Code Quality ObservationspageLifecycle.ts ChangesGood:
Potential Issue - Race Condition: document.addEventListener('readystatechange', function onReadyStateChange() {
document.removeEventListener('readystatechange', onReadyStateChange);
onPageReady(callback); // ⚠️ Recursive call
});Concern: If Suggested fix: document.addEventListener('readystatechange', function onReadyStateChange() {
if (document.readyState === 'complete') {
document.removeEventListener('readystatechange', onReadyStateChange);
if (!isPageLifecycleInitialized) {
isPageLifecycleInitialized = true;
callback();
}
}
});Test ChangesGood:
Observation:
This is the intended change and makes sense for preventing race conditions. Integration Test CleanupGood:
Question:
🐛 Potential Issues
🔒 Security ConsiderationsNo security concerns identified. The changes are purely related to timing of hydration. ⚡ Performance Considerations
📋 Testing Recommendations
🎯 RecommendationsHigh Priority
Medium Priority
Low Priority
📝 SummaryVerdict: This is a necessary fix for a critical race condition, and the approach is sound. However, there's a potential edge case in the Impact:
The core solution is correct, but the implementation needs a small refinement to handle rapid state transitions properly. Overall: Approve with minor changes requested (fix the race condition in the event listener). |
Code Review: PR #1988 - Only initiate hydration after all subresources loadedThank you @Judahmeek for this PR addressing component registration race conditions! SummaryThis PR changes the page lifecycle to wait for ✅ Strengths
🔴 Critical Issues1. Logic Bug: Callback Won't Execute (
|
Code Review for PR #1988: Only initiate hydration after all subresources loadedThank you for working on this critical race condition fix, @Judahmeek! I've reviewed the changes and have identified several important issues that need to be addressed. 🎯 SummaryThis PR changes page lifecycle initialization to wait for 🔴 Critical Issues1. Logic Bug: Callback Never Executes Due to Recursive GuardLocation: The current implementation has a critical flaw where the callback will never execute when called recursively: let isPageLifecycleInitialized = false;
function onPageReady(callback: () => void) {
if (typeof window === 'undefined') return;
if (document.readyState === 'complete') {
if (isPageLifecycleInitialized) {
return; // ❌ Returns WITHOUT calling callback!
}
isPageLifecycleInitialized = true;
callback();
} else {
document.addEventListener('readystatechange', function onReadyStateChange() {
document.removeEventListener('readystatechange', onReadyStateChange);
onPageReady(callback); // ⚠️ Calls itself recursively
});
}
}Problem Flow:
Wait, I'm re-reading the diff. Let me check the actual code in the PR diff again: Looking at the diff more carefully: +function onPageReady(callback: () => void) {
+ if (typeof window === 'undefined') return;
+
+ if (document.readyState === 'complete') {
+ if (isPageLifecycleInitialized) {
+ return;
+ }
+ isPageLifecycleInitialized = true;
+ callback();
+ } else {
+ document.addEventListener('readystatechange', function onReadyStateChange() {
+ document.removeEventListener('readystatechange', onReadyStateChange);
+ onPageReady(callback);
+ });
+ }
+}The issue: The guard
Recommended Fix: let isPageLifecycleInitialized = false;
function onPageReady(callback: () => void) {
if (typeof window === 'undefined') return;
if (isPageLifecycleInitialized) {
return; // Already initialized, don't add more listeners
}
if (document.readyState === 'complete') {
isPageLifecycleInitialized = true;
callback();
} else {
// Only add listener once
const onReadyStateChange = () => {
if (document.readyState === 'complete') {
document.removeEventListener('readystatechange', onReadyStateChange);
isPageLifecycleInitialized = true;
callback();
}
};
document.addEventListener('readystatechange', onReadyStateChange);
}
}2. Performance Impact: Significant Delay in HydrationConcern: Waiting for
Previous behavior: Impact: Users will experience a noticeable delay before React components become interactive Questions:
Suggestion: Consider making this configurable: // In ReactOnRails configuration
{
waitForAllResources: false, // default
// or
waitForAllResources: true // opt-in for apps with component registration issues
}
|
Code Review: PR #1988 - Only initiate hydration after all subresources loadedOverviewThis PR addresses a component registration race condition by changing when React hydration begins - from ✅ Strengths
🚨 Critical Issues1. Logic Bug: Callback Never Executes on Recursive Calllet isPageLifecycleInitialized = false;
function onPageReady(callback: () => void) {
if (typeof window === 'undefined') return;
if (document.readyState === 'complete') {
if (isPageLifecycleInitialized) {
return; // ❌ Returns WITHOUT calling callback!
}
isPageLifecycleInitialized = true;
callback();
} else {
document.addEventListener('readystatechange', function onReadyStateChange() {
document.removeEventListener('readystatechange', onReadyStateChange);
onPageReady(callback); // ❌ Recursive call hits the guard above!
});
}
}Problem: When
Suggested Fix: let isPageLifecycleInitialized = false;
function onPageReady(callback: () => void) {
if (typeof window === 'undefined') return;
if (isPageLifecycleInitialized) {
return;
}
if (document.readyState === 'complete') {
isPageLifecycleInitialized = true;
callback();
} else {
document.addEventListener('readystatechange', function onReadyStateChange() {
if (document.readyState === 'complete') {
document.removeEventListener('readystatechange', onReadyStateChange);
isPageLifecycleInitialized = true;
callback();
}
});
}
}This checks the guard before state checks and directly executes the callback in the listener rather than recursing. 2. Missing Initialization Guard Leads to Duplicate ListenersThe original code had
This results in duplicate event listeners for Turbo/Turbolinks events and multiple calls to
|
Code Review: PR #1988 - Only initiate hydration after all subresources loadedThank you for this PR! I've completed a thorough review. The approach correctly addresses the component registration race condition, but there are some critical implementation issues that need to be fixed before merging. 🎯 SummaryThis PR changes page lifecycle initialization from waiting for ✅ Strengths
🔴 Critical Issues1. Recursive Logic Bug (pageLifecycle.ts:62-76)Problem: The recursive call pattern is fragile and confusing: function onPageReady(callback: () => void) {
if (typeof window === 'undefined') return;
if (document.readyState === 'complete') {
if (isPageLifecycleInitialized) {
return; // ⚠️ Returns WITHOUT calling callback
}
isPageLifecycleInitialized = true;
callback();
} else {
document.addEventListener('readystatechange', function onReadyStateChange() {
document.removeEventListener('readystatechange', onReadyStateChange);
onPageReady(callback); // ⚠️ Recursive call AFTER removeEventListener
});
}
}Issues:
Impact: While it may work, the logic is error-prone and could break with future changes. Recommended Fix: function onPageReady(callback: () => void) {
if (typeof window === 'undefined') return;
if (isPageLifecycleInitialized) {
return;
}
if (document.readyState === 'complete') {
isPageLifecycleInitialized = true;
callback();
} else {
const onReadyStateChange = () => {
if (document.readyState === 'complete') {
document.removeEventListener('readystatechange', onReadyStateChange);
isPageLifecycleInitialized = true;
callback();
}
};
document.addEventListener('readystatechange', onReadyStateChange);
}
}Benefits of this approach:
2. Test Naming Inconsistency (pageLifecycle.test.js:84)it('should wait for readystatechange when document.readyState is "interactive"', () => {
setReadyState('interactive'); // Changed from 'loading' in the diffProblem: The test was originally for "interactive" but the diff shows it's testing with Issue: This is actually incorrect behavior! When
But the test name and expectation mismatch the actual behavior shown in your CURRENT pageLifecycle.ts file (lines 70-71 in my reading), which would run immediately for Action Required:
3. Test Coverage GapThe updated tests verify that:
But they don't verify:
Recommended Addition: it('should execute callback when readyState becomes complete', () => {
setReadyState('loading');
const callback = jest.fn();
const { onPageLoaded } = importPageLifecycle();
onPageLoaded(callback);
expect(callback).not.toHaveBeenCalled();
// Simulate readyState change
setReadyState('complete');
const listener = addEventListenerSpy.mock.calls.find(
call => call[0] === 'readystatechange'
)[1];
listener();
expect(callback).toHaveBeenCalledTimes(1);
});
|
Code Review for PR #1988OverviewThis PR changes the hydration timing from DOMContentLoaded to window.load (document.readyState === 'complete') to prevent race conditions when component registration happens asynchronously. This is a reasonable approach given that async hydration (CallbackRegistry) was moved to Pro. Critical IssuesCRITICAL BUG: Infinite Recursion RiskLocation: packages/react-on-rails/src/pageLifecycle.ts:62-78 The new onPageReady function has a potential infinite recursion issue. The readystatechange event fires for BOTH 'interactive' AND 'complete' states. When the listener fires at 'interactive', it recursively calls onPageReady(), which will add ANOTHER readystatechange listener, causing unnecessary event listener churn. Fix: Check the readyState before recursing, or use the traditional window.load event instead. Test Coverage IssueLocation: packages/react-on-rails/tests/pageLifecycle.test.js:84-108 The test for 'interactive' state has incorrect comments and doesn't verify the correctness of the recursion logic. It should simulate the readystatechange event firing and verify that the callback is eventually called when readyState becomes 'complete'. Removed Test CoverageLocation: packages/react-on-rails/tests/pageLifecycle.test.js:235-256 (deleted) The preventing duplicate initialization test was removed. You should verify that duplicate initialization is still prevented with the new implementation. The isPageLifecycleInitialized flag is checked inside the readyState === 'complete' branch, which means multiple calls to onPageLoaded/onPageUnloaded before the page is ready will add multiple readystatechange listeners. Design ConcernsPerformance: Delayed HydrationImpact: Components will now hydrate LATER - after all subresources (images, stylesheets, fonts, etc.) are loaded instead of after DOM parsing completes. Trade-offs:
Suggestion: Consider documenting this trade-off in the changelog/docs. Users migrating from Pro to open-source may notice their apps feel slower. Semantic Change: DOMContentLoaded to window.loadThe PR uses readystatechange checking for 'complete' instead of the more explicit window.load event. Both fire at the same time, but window.load is more explicit about intent. Code QualityGood:
Needs Improvement:
Integration Test RemovalTwo test contexts were removed from spec/dummy/spec/system/integration_spec.rb. This is correct since these tests were specifically for the Pro feature (immediate_hydration) that was moved to the Pro package. Verify these tests exist in react_on_rails_pro/ test suite. RecommendationsPriority 1 (Must Fix):
Priority 2 (Should Consider): Priority 3 (Nice to Have): SummaryConcept: Sound approach to fix race conditions Recommendation: Request changes to fix the recursion issue before merging. |
Code Review: PR #1988 - Only initiate hydration after all subresources loadedOverviewThis PR changes the page lifecycle initialization logic to wait for the complete document state (all subresources loaded) instead of DOMContentLoaded (DOM parsed). This aims to prevent component registration race conditions by ensuring all JavaScript bundles are fully loaded before attempting hydration. Critical Issues🔴 1. Significant Behavior Change Without Sufficient TestingIssue: The PR changes from
Impact: This could significantly delay component hydration, especially on pages with large images or slow-loading resources. The delay could be noticeable to users. Evidence: The test changes show this is intentional (line 97 comment change from "interactive" to "loading"), but there's no performance testing or measurement of the impact. Recommendation:
🟡 2. Removed Test Coverage for Duplicate InitializationIssue: The PR removes the "preventing duplicate initialization" test (lines 236-255 in the old test file) that verified Problem: The new implementation still uses this flag (pageLifecycle.ts:61-62, 65-68), but now lacks test coverage. The logic changed subtly - the flag is now checked INSIDE the Risk: Without tests, future refactors could break this behavior and cause memory leaks from duplicate event listeners. Recommendation: Add back a test that verifies: it('should not initialize listeners multiple times when document is complete', () => {
setReadyState('complete');
const { onPageLoaded } = importPageLifecycle();
onPageLoaded(jest.fn());
onPageLoaded(jest.fn());
// Verify setupPageNavigationListeners is only called once
expect(addEventListenerSpy).toHaveBeenCalledTimes(0); // No nav library
// But both callbacks should be registered
});🟡 3. Incorrect Test Description and Expectation (Line 84-95)Issue: The test at line 84 says "should wait for readystatechange when document.readyState is 'interactive'" but the test actually verifies that callbacks DON'T run immediately when state is 'interactive'. Problem: According to MDN, The new code treats 'interactive' same as 'loading' - it waits for 'complete'. This is MORE conservative than before. Current behavior: if (document.readyState === 'complete') { // Only 'complete' runs immediately
callback();
} else { // 'loading' AND 'interactive' both wait
addEventListener('readystatechange', ...);
}Recommendation: This might be intentional, but:
Correctness Issues🟡 4. Missing
|
Code Review for PR #1988SummaryThis PR reverts hydration timing to wait for Positive Aspects ✅
Issues & Concerns
|
Code Review: Only initiate hydration after all subresources loadedThank you for this PR! I've reviewed the changes and here's my comprehensive feedback: 📊 OverviewPurpose: Change page lifecycle initialization from Files Changed:
✅ Strengths
|
|
Closed as shouldn't be necessary if all deferred javascript runs before the DOMContentLoaded event, ass specifications insist. |
Since async hydration is only supported by ReactOnRails Pro now that the Callback Registry has been moved to ReactOnRails Pro only, we need to either revert back to ReactOnRails only hydrating after all subresources are loaded or return the CallbackRegistry & async support back to ReactOnRails in order to prevent component registration race conditions like this one.
This change is
Summary by CodeRabbit
Refactor
Tests