Remove gap in page lifecycle check for setup listeners #1771

Judahmeek · 2025-09-09T23:01:27Z

We currently have a gap between the DOMContentLoaded event & readystate === 'complete' (which is after the load event), where this script could run and NOTHING happen.

This change is

Summary by CodeRabbit

Performance Improvements
- Page navigation listeners now initialize earlier when the document is interactive, reducing startup latency and improving responsiveness after page load.
Bug Fixes
- Prevents delayed setup that could miss early user interactions on fast-loading pages by ensuring listeners attach as soon as the DOM is ready.

We currently have a gap between the DOMContentLoaded event & readystate === 'complete' (which is after the load event), where this script could run and NOTHING happen.

coderabbitai · 2025-09-09T23:01:39Z

Walkthrough

Adjusted initializePageEventListeners to treat any document.readyState other than 'loading' as ready, invoking setupPageNavigationListeners immediately in 'interactive' or 'complete'. Retained DOMContentLoaded listener only for the 'loading' state. No public API changes.

Changes

Cohort / File(s)	Summary
Lifecycle initialization logic `node_package/src/pageLifecycle.ts`	Changed readiness check from `readyState === 'complete'` to `readyState !== 'loading'`, causing immediate setup during 'interactive' and 'complete'; kept DOMContentLoaded fallback for 'loading'.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant B as Browser
  participant L as pageLifecycle
  participant N as setupPageNavigationListeners

  B->>L: initializePageEventListeners()
  alt document.readyState !== 'loading' (interactive/complete)
    Note over L: New behavior triggers immediately for 'interactive' too
    L->>N: setup()
  else document.readyState === 'loading'
    L-->>B: addEventListener('DOMContentLoaded', ...)
    B-->>L: DOMContentLoaded
    L->>N: setup()
  end

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title succinctly captures the primary intent of the changeset—eliminating the timing gap in page lifecycle checks for setup listeners—using clear and specific language without unnecessary detail.
Description Check	✅ Passed	The description directly addresses the timing gap issue between DOMContentLoaded and the complete readyState that the changeset resolves, making it clearly related to the modifications proposed.

Poem

I thump my paws: the page is near,
From loading fog to states more clear.
Interactive? I hop right in—
No need to wait, let nav begin.
When DOM’s done baking, I still arrive,
A punctual rabbit, ears alive.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

✨ Finishing Touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch Judahmeek-patch-1

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

node_package/src/pageLifecycle.ts (1)
70-74: Use a one-shot DOMContentLoaded listener to avoid a lingering handler.

Make the fallback listener self-removing.
   } else {
-    document.addEventListener('DOMContentLoaded', setupPageNavigationListeners);
+    document.addEventListener('DOMContentLoaded', setupPageNavigationListeners, { once: true });
   }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f4adcd9 and f113a2a.

📒 Files selected for processing (1)

node_package/src/pageLifecycle.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

node_package/src/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

JavaScript/TypeScript code must adhere to ESLint rules (project uses ESLint for JS/TS linting)

Files:

node_package/src/pageLifecycle.ts

node_package/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Client-side source should be authored in TypeScript under node_package/src (compiled to node_package/lib)

Files:

node_package/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). (5)

GitHub Check: dummy-app-integration-tests (newest)
GitHub Check: dummy-app-integration-tests (oldest)
GitHub Check: build
GitHub Check: rspec-package-tests (oldest)
GitHub Check: rspec-package-tests (newest)

🔇 Additional comments (2)

node_package/src/pageLifecycle.ts (2)

70-74: Good fix: close the 'interactive' gap.

Switching to document.readyState !== 'loading' correctly handles the window between DOMContentLoaded and load, so listeners are set up when the doc is interactive or complete.

70-74: Verify Turbo/Turbolinks availability before attaching listeners. Confirm our integration guarantees that Turbo/Turbolinks scripts are loaded by the time DOMContentLoaded/interactive fires; if not, add a minimal fallback (e.g. re-check on window.load with guards to avoid duplicate installs).

Judahmeek · 2025-09-10T00:41:43Z

Superseded by #1773

Remove gap in page lifecycle check for setup listeners

f113a2a

We currently have a gap between the DOMContentLoaded event & readystate === 'complete' (which is after the load event), where this script could run and NOTHING happen.

coderabbitai bot reviewed Sep 9, 2025

View reviewed changes

Judahmeek closed this Sep 10, 2025

Judahmeek deleted the Judahmeek-patch-1 branch September 10, 2025 00:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Remove gap in page lifecycle check for setup listeners #1771

Remove gap in page lifecycle check for setup listeners #1771

Uh oh!

Judahmeek commented Sep 9, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Sep 9, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Judahmeek commented Sep 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Remove gap in page lifecycle check for setup listeners #1771

Remove gap in page lifecycle check for setup listeners #1771

Uh oh!

Conversation

Judahmeek commented Sep 9, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Pre-merge checks (2 passed, 1 warning)

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Judahmeek commented Sep 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Judahmeek commented Sep 9, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Sep 9, 2025 •

edited

Loading