Fix initial page startup race for late-loading client bundles#3151
Fix initial page startup race for late-loading client bundles#3151
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughInitialize logic now defers setup during Changes
Sequence Diagram(s)sequenceDiagram
participant Client as ClientBundle
participant Doc as document
participant Setup as setupPageNavigationListeners()
Client->>Doc: check readyState
alt readyState === "complete"
Client->>Setup: run setup immediately
Setup-->>Client: done
else
Client->>Doc: addEventListener("DOMContentLoaded", handler)
alt readyState === "interactive" (initial)
Client->>Doc: addEventListener("readystatechange", readyStateHandler)
end
Note over Doc,Setup: One of the handlers fires
Doc-->>Setup: DOMContentLoaded or readystatechange->complete
Setup-->>Client: run setup (idempotent)
Setup->>Doc: removeEventListener("DOMContentLoaded", handler)
Setup->>Doc: removeEventListener("readystatechange", readyStateHandler)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
size-limit report 📦
|
ihabadham
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| export function consumeInitialPageLoadIfNeeded(): boolean { | ||
| if (currentPageState !== 'initial') { | ||
| return false; | ||
| } | ||
|
|
||
| if (document.readyState === 'complete') { | ||
| cleanupInitialPageLoadListeners(); | ||
| ensureNavigationListenersInstalled(); | ||
| } | ||
|
|
||
| runPageLoadedCallbacks(); | ||
| return true; |
There was a problem hiding this comment.
There's an asymmetry here compared to handleAutomaticInitialPageLoad: when readyState is not 'complete', this function calls runPageLoadedCallbacks() without first calling ensureNavigationListenersInstalled(). Navigation listeners (Turbo/Turbolinks) are deferred until the automatic readystatechange→complete path fires later.
This is intentional (the "defers turbo listener" test covers it), but the consequence is a narrow window where Turbo navigation events fired between this manual call and the complete transition would be missed. A brief comment here would help future readers understand the deliberate deferral.
Also worth noting: when called during interactive after DOMContentLoaded has already fired, the initialPageLoadReadyHandler registered in initializePageEventListeners() is a dead listener — DOMContentLoaded will never fire again. It stays registered in memory until the readystatechange handler fires and triggers cleanupInitialPageLoadListeners(). It's harmless, but it's worth a comment.
| export function consumeInitialPageLoadIfNeeded(): boolean { | |
| if (currentPageState !== 'initial') { | |
| return false; | |
| } | |
| if (document.readyState === 'complete') { | |
| cleanupInitialPageLoadListeners(); | |
| ensureNavigationListenersInstalled(); | |
| } | |
| runPageLoadedCallbacks(); | |
| return true; | |
| export function consumeInitialPageLoadIfNeeded(): boolean { | |
| if (currentPageState !== 'initial') { | |
| return false; | |
| } | |
| // Only install navigation listeners and clean up the pending DOM handlers if we are | |
| // already at 'complete'. If we're still at 'interactive', defer listener installation | |
| // until the automatic readystatechange→complete path fires (see initialPageLoadCompleteHandler | |
| // in initializePageEventListeners). This intentionally leaves a narrow window where Turbo | |
| // navigation events fired before 'complete' would be missed, but that window is negligible | |
| // in practice. | |
| if (document.readyState === 'complete') { | |
| cleanupInitialPageLoadListeners(); | |
| ensureNavigationListenersInstalled(); | |
| } | |
| runPageLoadedCallbacks(); | |
| return true; | |
| } |
| export function reactOnRailsPageLoaded() { | ||
| const startupWasPending = clientStartup(); | ||
| if (startupWasPending) { | ||
| consumeInitialPageLoadIfNeeded(); | ||
| return; |
There was a problem hiding this comment.
There's a silent third case: startupWasPending = true but consumeInitialPageLoadIfNeeded() returns false. This can happen if currentPageState is already 'load' when reactOnRailsPageLoaded() is called for the first time (e.g., onPageLoaded already advanced the state before this function ran). In that case, onPageLoaded(runAutomaticPageLoad) inside clientStartup() immediately invokes runAutomaticPageLoad (because onPageLoaded fast-paths when state is 'load'), so the render already happened and no further action is needed.
This is correct but non-obvious — a comment here would help:
| export function reactOnRailsPageLoaded() { | |
| const startupWasPending = clientStartup(); | |
| if (startupWasPending) { | |
| consumeInitialPageLoadIfNeeded(); | |
| return; | |
| const startupWasPending = clientStartup(); | |
| if (startupWasPending) { | |
| // clientStartup() just registered runAutomaticPageLoad via onPageLoaded. | |
| // If the page lifecycle is already at 'load', onPageLoaded() invoked it immediately, | |
| // so consumeInitialPageLoadIfNeeded() will return false and we're done. | |
| // If the page lifecycle is still at 'initial', consumeInitialPageLoadIfNeeded() | |
| // will advance it and trigger the render now. | |
| consumeInitialPageLoadIfNeeded(); | |
| return; | |
| } |
| document.addEventListener('DOMContentLoaded', initialPageLoadReadyHandler); | ||
|
|
||
| if (document.readyState === 'interactive') { | ||
| document.addEventListener('readystatechange', initialPageLoadCompleteHandler); | ||
| } |
There was a problem hiding this comment.
The readystatechange listener is only registered when readyState === 'interactive', not for 'loading'. This is correct — the "missed DOMContentLoaded" scenario only arises when the bundle starts during interactive. But a comment spelling this out would help:
| document.addEventListener('DOMContentLoaded', initialPageLoadReadyHandler); | |
| if (document.readyState === 'interactive') { | |
| document.addEventListener('readystatechange', initialPageLoadCompleteHandler); | |
| } | |
| document.addEventListener('DOMContentLoaded', initialPageLoadReadyHandler); | |
| // Only register the readystatechange fallback during 'interactive'. During 'loading', | |
| // DOMContentLoaded is guaranteed to fire after deferred scripts run, so no fallback | |
| // is needed. The fallback is specifically for the case where the bundle begins executing | |
| // during 'interactive' after DOMContentLoaded has already fired. | |
| if (document.readyState === 'interactive') { | |
| document.addEventListener('readystatechange', initialPageLoadCompleteHandler); | |
| } |
| delete globalThis.__REACT_ON_RAILS_EVENT_HANDLERS_RAN_ONCE__; | ||
| }); | ||
|
|
||
| it('does not render twice when manual page load runs during interactive before deferred startup registration', async () => { |
There was a problem hiding this comment.
The test name mentions "before deferred startup registration" but the actual scenario being tested is that reactOnRailsPageLoaded() is called before the setTimeout(() => clientStartup(), 0) in ReactOnRails.client.ts fires. Consider renaming for clarity:
| it('does not render twice when manual page load runs during interactive before deferred startup registration', async () => { | |
| it('does not render twice when manual reactOnRailsPageLoaded() is called before the deferred clientStartup() setTimeout fires', async () => { |
| setReadyState('complete'); | ||
| document.dispatchEvent(new Event('readystatechange')); | ||
| await waitForNextTick(); | ||
|
|
||
| expect(counts).toEqual({ storeCalls: 1, rendererCalls: 1 }); |
There was a problem hiding this comment.
This dispatches the real readystatechange event via document.dispatchEvent, which will call the actual initialPageLoadCompleteHandler registered in initializePageEventListeners. The test correctly verifies the full end-to-end path, but note that addEventListenerSpy is not mocked here (it's a spy, not a stub), so the real listeners are actually registered and called. This is a good integration-level test.
One missing assertion: after the readystatechange fires, it might be worth verifying that the handlers are cleaned up (i.e., removeEventListener was called for both DOMContentLoaded and readystatechange). Otherwise a future regression that forgets to clean up listeners would slip past this test.
Code ReviewOverviewThis PR fixes a real browser timing gap: when a bundle starts executing during The logic is correct and the test coverage is solid. A few things worth discussing: Logic / Correctness
When called during The implied consequence: if a Turbo navigation fires between the manual call and the document reaching Dead When Silent third case in When Test CoverageGood regression coverage for the two new scenarios. A few gaps:
Minor
SummaryThe core fix is sound and the approach is the right one. The main ask is additional comments in |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae74608764
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const startupWasPending = clientStartup(); | ||
| if (startupWasPending) { | ||
| consumeInitialPageLoadIfNeeded(); | ||
| return; |
There was a problem hiding this comment.
Calling clientStartup() unconditionally at the start of reactOnRailsPageLoaded() changes option timing for async bundles that execute after document.readyState === "complete". In that flow, onPageLoaded(...) initializes immediately and locks navigation listener setup before a later ReactOnRails.setOptions({ turbo: true }) call in the same tick, and the deferred startup timer is then skipped because __REACT_ON_RAILS_EVENT_HANDLERS_RAN_ONCE__ is already set. The result is that Turbo listeners are never installed, so subsequent Turbo navigations miss unload/load lifecycle callbacks. Before this change, deferred startup still ran after setOptions and would install the listeners.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-on-rails/tests/clientStartup.test.js (1)
118-137: Consider whethersetOptionstiming reflects real usage.The test sets
turbo: trueafterreactOnRailsPageLoaded()completes. In practice, wouldn't users typically configure options before the page load runs?If the intent is to verify that Turbo listeners are installed regardless of when the option is set (as long as it's before
complete), this is fine. Otherwise, consider movingsetOptions({ turbo: true })before thereactOnRailsPageLoaded()call to better match real-world usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails/tests/clientStartup.test.js` around lines 118 - 137, The test currently calls ReactOnRails.setOptions({ turbo: true }) after ReactOnRails.reactOnRailsPageLoaded(), which doesn't match typical usage; update the test "defers turbo listener installation..." to call ReactOnRails.setOptions({ turbo: true }) before invoking ReactOnRails.reactOnRailsPageLoaded() (or alternatively add a new test that asserts behavior when options are set post-load), so the setup reflects real-world ordering and still verifies turbo listener installation on 'complete'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-on-rails/tests/clientStartup.test.js`:
- Around line 118-137: The test currently calls ReactOnRails.setOptions({ turbo:
true }) after ReactOnRails.reactOnRailsPageLoaded(), which doesn't match typical
usage; update the test "defers turbo listener installation..." to call
ReactOnRails.setOptions({ turbo: true }) before invoking
ReactOnRails.reactOnRailsPageLoaded() (or alternatively add a new test that
asserts behavior when options are set post-load), so the setup reflects
real-world ordering and still verifies turbo listener installation on
'complete'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69607a43-76e4-478a-9bf8-0658976b9d0e
📒 Files selected for processing (4)
packages/react-on-rails/src/clientStartup.tspackages/react-on-rails/src/pageLifecycle.tspackages/react-on-rails/tests/clientStartup.test.jspackages/react-on-rails/tests/pageLifecycle.test.js
Greptile SummaryThis PR fixes a browser timing gap where the React on Rails client startup misses the initial page load when a bundle begins executing during Confidence Score: 5/5Safe to merge; all findings are P2 style or precision notes that don't affect correctness in practice. The core logic is sound and well-tested. The three-branch initialization (complete/interactive/loading) correctly handles the missed-DOMContentLoaded window. Double-render prevention is verified by integration tests. Remaining issues are a missing return type annotation and a harmless spurious packages/react-on-rails/src/pageLifecycle.ts — review the deferred navigation listener window comment and the spurious removeEventListener note. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Bundle executes] --> B{readyState?}
B -->|complete| C[handleAutomaticInitialPageLoad\nimmediate path]
B -->|interactive| D[Add DOMContentLoaded listener\nAdd readystatechange listener NEW]
B -->|loading| E[Add DOMContentLoaded listener only]
C --> F[ensureNavigationListenersInstalled]
F --> G{turbolinks2?}
G -->|no| H[runPageLoadedCallbacks\nrenders components]
G -->|yes| I[Wait for page:change event]
D --> J{DOMContentLoaded fires?}
J -->|yes, caught| K[handleAutomaticInitialPageLoad\nnormal path]
J -->|already missed| L[readystatechange → complete\nrecovery path NEW]
K --> F
L --> M[handleAutomaticInitialPageLoad\ncurrentPageState!=initial branch]
M --> N[ensureNavigationListenersInstalled\ncleanup listeners]
E --> O[DOMContentLoaded fires] --> K
subgraph Manual ["reactOnRailsPageLoaded() manual call"]
P[clientStartup] -->|first run, returns true| Q[consumeInitialPageLoadIfNeeded]
P -->|already ran, returns false| R{currentPageState?}
Q -->|readyState=complete| S[cleanup + install listeners\n+ runPageLoadedCallbacks]
Q -->|readyState=interactive| T[runPageLoadedCallbacks\ndefer nav listeners to readystatechange]
R -->|initial| S
R -->|load| U[runAutomaticPageLoad\ndirect re-render]
end
Reviews (1): Last reviewed commit: "Prevent duplicate page-load sweeps durin..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return true; | ||
| } | ||
|
|
||
| export function reactOnRailsPageLoaded() { |
There was a problem hiding this comment.
reactOnRailsPageLoaded is the only exported function in this file without an explicit return type, making it inconsistent with clientStartup(): boolean just above it.
| export function reactOnRailsPageLoaded() { | |
| export function reactOnRailsPageLoaded(): void { |
| export function consumeInitialPageLoadIfNeeded(): boolean { | ||
| if (currentPageState !== 'initial') { | ||
| return false; | ||
| } | ||
|
|
||
| if (document.readyState === 'complete') { | ||
| cleanupInitialPageLoadListeners(); | ||
| ensureNavigationListenersInstalled(); | ||
| } | ||
|
|
||
| runPageLoadedCallbacks(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
runPageLoadedCallbacks() during interactive
When consumeInitialPageLoadIfNeeded is called while readyState === 'interactive' (the deferred-startup recovery path), the function skips cleanupInitialPageLoadListeners / ensureNavigationListenersInstalled and calls runPageLoadedCallbacks() immediately. Navigation listeners (Turbo turbo:render, Turbolinks events) are deferred until the subsequent readystatechange → complete transition.
This is intentional and tested, but it creates a narrow window during the interactive → complete gap where a Turbo navigation would fire turbo:before-render / turbo:render without any listener installed, leaving components neither unmounted nor remounted. In practice a Turbo navigation during page-load is rare, but it may be worth adding a comment here so future readers understand why navigation listeners aren't installed at this call site.
| function cleanupInitialPageLoadListeners(): void { | ||
| if (initialPageLoadReadyHandler) { | ||
| document.removeEventListener('DOMContentLoaded', initialPageLoadReadyHandler); | ||
| initialPageLoadReadyHandler = null; | ||
| } | ||
|
|
||
| if (initialPageLoadCompleteHandler) { | ||
| document.removeEventListener('readystatechange', initialPageLoadCompleteHandler); | ||
| initialPageLoadCompleteHandler = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
removeEventListener call when started in loading state
initialPageLoadCompleteHandler is always assigned in the else branch of initializePageEventListeners, but the readystatechange listener is only registered when readyState === 'interactive'. When startup happens during loading, cleanupInitialPageLoadListeners will call document.removeEventListener('readystatechange', initialPageLoadCompleteHandler) for a listener that was never added — a harmless no-op, but slightly imprecise. Consider guarding with the same readyState === 'interactive' check, or only assigning initialPageLoadCompleteHandler when the listener is actually registered.
## Summary - Applies `prettier --write` to five tracked docs files that are currently out of compliance on `main`. - No content changes — only whitespace/table-alignment fixes produced by Prettier. ## Why this is needed The formatting drift landed via a docs-only merge to `main`. The `lint-js-and-ruby.yml` workflow is conditionally skipped for docs-only commits on `main` (see the job condition at [`.github/workflows/lint-js-and-ruby.yml:79-87`](https://github.com/shakacode/react_on_rails/blob/main/.github/workflows/lint-js-and-ruby.yml#L79-L87)), so the check never ran on merge. It surfaces on every subsequent code-touching PR because the lint job runs for those (e.g. [#3151](#3151), [#3148](#3148), [#3142](#3142), [#3097](#3097)). Merging this unblocks the `build` → `Check formatting` step on those PRs. ## Test plan - [x] `pnpm start format.listDifferent` passes for the five files - [x] Diff is purely whitespace — no link targets, code blocks, or prose changed 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Improved formatting across API reference, core concepts, migration, and RSC docs: refined table layouts, adjusted column alignment, and standardized emphasis for clearer warnings. * Updated link text and phrasing for several guides for clearer navigation. * Reformatted code examples and JSX/Route snippets for more compact, consistent presentation and improved readability. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This reverts commit ae74608.
ihabadham
force-pushed
the
fix/page-lifecycle-complete-fallback
branch
from
ae74608 to
05e43fb
Compare
Code ReviewOverviewThis PR fixes a real browser timing window: when the client bundle begins executing while The logic is correct and the approach is well-suited to the constraint (can't initialize early during What works well
Issues1. Missing negative assertion for the
|
| expect(callback).not.toHaveBeenCalled(); | ||
| // Verify that a DOMContentLoaded listener was added when readyState is 'interactive' | ||
| expect(addEventListenerSpy).toHaveBeenCalledWith('DOMContentLoaded', expect.any(Function)); | ||
| expect(addEventListenerSpy).toHaveBeenCalledWith('readystatechange', expect.any(Function)); |
There was a problem hiding this comment.
Good addition here. The symmetric negative assertion is missing from the loading state test directly below — consider adding:
// in the 'loading' test:
expect(addEventListenerSpy).not.toHaveBeenCalledWith('readystatechange', expect.any(Function));Since readystatechange is the distinguishing part of this fix, a negative assertion in the loading path makes the intent explicit and guards against accidentally adding it there in the future.
| let setupPageNavigationListenersOnce: () => void; | ||
| let setupPageNavigationListenersOnComplete: () => void; | ||
|
|
||
| const cleanupListeners = (): void => { |
There was a problem hiding this comment.
cleanupListeners closes over setupPageNavigationListenersOnce and setupPageNavigationListenersOnComplete, but both are still undefined at this point — they're only assigned on the lines that follow. This is safe at call time (the closures are never invoked before the assignments complete), but it's a subtle pattern that can trip up readers unfamiliar with JS closure/TDZ semantics.
One way to make it more self-evident: move cleanupListeners to after the two setupPageNavigation… assignments, or restructure so the function references are captured inline at the addEventListener call sites rather than through shared let bindings.
| const importPageLifecycle = () => require('../src/pageLifecycle.ts'); | ||
|
|
||
| const getEventHandler = (eventName) => | ||
| addEventListenerSpy.mock.calls.find((call) => call[0] === eventName)?.[1]; |
There was a problem hiding this comment.
Array.find returns the first matching call. If a future test registers multiple listeners for the same event name, this helper silently returns only the first one and the second would be untestable via this API. It's low-risk for the current test suite, but worth noting. Using findLast (or asserting on mock.calls.filter(...).length) for ordered scenarios would be more robust.
Collapse the two closures + forward declarations + isSetupComplete flag into a single handler registered for both DOMContentLoaded and readystatechange. The handler filters readystatechange events by readyState and removes both listeners in one shot before running setup. The flag guarded against a scenario that cannot occur in real browsers: removeEventListener is synchronous, so once the first event is handled and both listeners are removed, the second event cannot re-invoke the handler. The same cleanup now lives directly in the handler. Update the "initialize once" test to verify the removeEventListener calls (the actual mechanism that prevents the double-run) instead of re-invoking handlers by reference, which bypassed the cleanup and misrepresented real browser behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review: Fix initial page startup race for late-loading client bundlesOverall: The fix is well-reasoned and targeted. The dual-listener strategy cleanly handles the missed-DOMContentLoaded window without breaking the existing deferred-script protection. A few items worth addressing before merge. What the fix does well
Issues1. Test for double-trigger prevention is incomplete (medium)The test "should remove both listeners after DOMContentLoaded fires so a later complete cannot re-trigger" (~line 379) asserts that 2.
|
| if (event.type === 'readystatechange' && document.readyState !== 'complete') return; | ||
| document.removeEventListener('DOMContentLoaded', initialPageLoadHandler); | ||
| document.removeEventListener('readystatechange', initialPageLoadHandler); | ||
| setupPageNavigationListeners(); |
There was a problem hiding this comment.
The idempotency here relies entirely on the two removeEventListener calls above succeeding before either browser event can re-enter this handler. That's safe in practice (single-threaded JS, well-behaved browsers), but setupPageNavigationListeners itself has no guard — if it were ever called a second time it would attach duplicate turbo:render / turbolinks:render listeners and double-fire every subsequent page-loaded callback.
Consider adding a one-line guard at the top of setupPageNavigationListeners:
| setupPageNavigationListeners(); | |
| setupPageNavigationListeners(); |
let isNavigationListenersSetUp = false;
function setupPageNavigationListeners(): void {
if (isNavigationListenersSetUp) return;
isNavigationListenersSetUp = true;
// … rest of functionThis makes the defence explicit rather than implicit.
| it('should remove both listeners after DOMContentLoaded fires so a later complete cannot re-trigger', () => { | ||
| setReadyState('interactive'); | ||
|
|
||
| const { onPageLoaded } = importPageLifecycle(); | ||
| const callback = jest.fn(); | ||
|
|
||
| onPageLoaded(callback); | ||
|
|
||
| const domContentLoadedHandler = getEventHandler('DOMContentLoaded'); | ||
| expect(domContentLoadedHandler).toBeDefined(); | ||
|
|
||
| domContentLoadedHandler(new Event('DOMContentLoaded')); | ||
| expect(callback).toHaveBeenCalledTimes(1); | ||
|
|
||
| // The handler's cleanup is what prevents the later readystatechange→complete | ||
| // event from firing the callback a second time in real browsers. | ||
| expect(removeEventListenerSpy).toHaveBeenCalledWith('DOMContentLoaded', expect.any(Function)); | ||
| expect(removeEventListenerSpy).toHaveBeenCalledWith('readystatechange', expect.any(Function)); | ||
| }); |
There was a problem hiding this comment.
This test verifies that removeEventListener is called, but since removeEventListenerSpy is mocked to a no-op (mockImplementation(() => {})), the handler is still attached in JSDOM. The comment below the assertions acknowledges the gap, but the test currently does not prove that the double-trigger is actually prevented.
A stronger version would simulate firing readystatechange after DOMContentLoaded has already run and assert the callback count is still 1:
it('should remove both listeners after DOMContentLoaded fires so a later complete cannot re-trigger', () => {
setReadyState('interactive');
const { onPageLoaded } = importPageLifecycle();
const callback = jest.fn();
onPageLoaded(callback);
// Restore real removeEventListener so cleanup actually works
removeEventListenerSpy.mockRestore();
const domContentLoadedHandler = getEventHandler('DOMContentLoaded');
const readystateChangeHandler = getEventHandler('readystatechange');
domContentLoadedHandler(new Event('DOMContentLoaded'));
expect(callback).toHaveBeenCalledTimes(1);
// Simulate the 'complete' transition that must not re-trigger
setReadyState('complete');
readystateChangeHandler(new Event('readystatechange'));
expect(callback).toHaveBeenCalledTimes(1); // still 1, not 2
});
| expect(callback).not.toHaveBeenCalled(); | ||
| // Verify that a DOMContentLoaded listener was added when readyState is 'interactive' | ||
| expect(addEventListenerSpy).toHaveBeenCalledWith('DOMContentLoaded', expect.any(Function)); | ||
| expect(addEventListenerSpy).toHaveBeenCalledWith('readystatechange', expect.any(Function)); |
There was a problem hiding this comment.
Good — this assertion documents that readystatechange is registered for interactive state. The symmetric negative assertion is missing from the adjacent loading-state test. Consider adding there:
// in the 'loading' state test, after the existing DOMContentLoaded assertion:
expect(addEventListenerSpy).not.toHaveBeenCalledWith('readystatechange', expect.any(Function));This documents the intentional asymmetry: the fallback listener is unnecessary when readyState === 'loading' because DOMContentLoaded cannot have already fired.
1 participant
Summary
Fixes a client lifecycle gap where React on Rails can miss the initial page startup if the bundle begins executing after
DOMContentLoadedhas already fired but before the document reachescomplete.Fixes #3150
Problem
Core startup currently initializes immediately only when
document.readyState === "complete", and otherwise waits forDOMContentLoaded.That avoids starting too early during
interactive, when deferred or module scripts may still be registering components.But there is another browser timing window:
document.readyState === "interactive"DOMContentLoadedhas already fired"complete"In that case, the
DOMContentLoadedlistener is attached too late, so the initial page-load callbacks never run.What changed
interactiveafterDOMContentLoadedhas already fired.ReactOnRails.reactOnRailsPageLoaded()calls with deferred startup so the same initial page load is not processed twice.Why this approach
The fix needs to handle both sides of the lifecycle correctly:
interactive, before deferred registration is finishedDOMContentLoadedwas already missed before startup attached its listenerTest plan
interactiveafterDOMContentLoadedhas already fired.ReactOnRails.reactOnRailsPageLoaded()during deferred startup so the initial page load is not processed twice.interactive.Summary by CodeRabbit