Improve naming clarity in pageLifecycle.ts #1762
Conversation
The main improvements:
- Rename setupTurbolinksEventListeners() to setupPageNavigationListeners()
to better reflect that it handles multiple navigation libraries, not just Turbolinks
- Rename isEventListenerInitialized to isPageLifecycleInitialized for clarity
- Extract hasNavigationLibrary variable to make the conditional logic clearer
- Update debug messages to be more descriptive ("DETECTED" vs "USING")
- Emphasize that the normal use case is NO navigation library
These changes make the code more self-documenting and easier to understand,
especially given that most users don't use Turbolinks at all.
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
WalkthroughRenamed setupTurbolinksEventListeners to setupPageNavigationListeners and isEventListenerInitialized to isPageLifecycleInitialized; unified navigation detection into hasNavigationLibrary; updated debug messages; when no navigation library is present, page-loaded callbacks run immediately; branch-specific event listener setup for Turbo, Turbolinks v5 and v2 remains. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant PageLifecycle
participant Turbo as Turbo (if present)
participant TL5 as Turbolinks v5 (if present)
participant TL2 as Turbolinks v2 (if present)
App->>PageLifecycle: initializePageEventListeners()
activate PageLifecycle
PageLifecycle->>PageLifecycle: detect hasNavigationLibrary
alt Turbo installed
Note over PageLifecycle,Turbo: setup turbo:before-render / turbo:render
PageLifecycle-->>Turbo: addEventListener(...)
else Turbolinks v5 installed
Note over PageLifecycle,TL5: setup turbolinks:before-render / turbolinks:render
PageLifecycle-->>TL5: addEventListener(...)
else Turbolinks v2 installed
Note over PageLifecycle,TL2: setup page:before-unload / page:change
PageLifecycle-->>TL2: addEventListener(...)
else No navigation library
Note over PageLifecycle: immediately run page-loaded callbacks
PageLifecycle-->>App: execute onPageLoaded callbacks
end
deactivate PageLifecycle
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
node_package/src/pageLifecycle.ts (4)
42-54: GuardturbolinksVersion5()withturbolinksInstalled()for robustness.Per our prior learning, always ensure Turbolinks exists before accessing its properties to avoid TypeErrors if assumptions change. Adding the explicit guard here makes the intent obvious even though
hasNavigationLibrarycurrently ensures safety.- } else if (turbolinksVersion5()) { + } else if (turbolinksInstalled() && turbolinksVersion5()) {Context: “when checking for Turbolinks v5 using
turbolinksVersion5(), ensureTurbolinksexists by checkingturbolinksInstalled()first.” (from retrieved learnings)
55-58: Verify initial callback for Turbolinks v2.Unlike the Turbo and TL v5 branches, TL v2 doesn’t invoke
runPageLoadedCallbacks()after attaching listeners. Ifpage:changedoesn’t reliably fire once on initial load in all setups, we may miss the first render.Option (safe, idempotent):
document.addEventListener('page:before-unload', runPageUnloadedCallbacks); document.addEventListener('page:change', runPageLoadedCallbacks); + if (currentPageState === 'initial') { + runPageLoadedCallbacks(); + }
73-73: Use{ once: true }for DOMContentLoaded listener.Auto-removes the handler and avoids retaining the reference.
- document.addEventListener('DOMContentLoaded', setupPageNavigationListeners); + document.addEventListener('DOMContentLoaded', setupPageNavigationListeners, { once: true });
37-37: Minor log phrasing consistency.To avoid “TURBO: TURBO DETECTED...” redundancy (given
debugTurbolinksprefixes logs) and to standardize tone:- debugTurbolinks('NO NAVIGATION LIBRARY: running page loaded callbacks immediately'); + debugTurbolinks('No navigation library detected: running page loaded callbacks now.'); - debugTurbolinks('TURBO DETECTED: adding event listeners for turbo:before-render and turbo:render.'); + debugTurbolinks('Detected Turbo: adding listeners for turbo:before-render and turbo:render.'); - 'TURBOLINKS 5 DETECTED: adding event listeners for turbolinks:before-render and turbolinks:render.', + 'Detected Turbolinks v5: adding listeners for turbolinks:before-render and turbolinks:render.', - debugTurbolinks('TURBOLINKS 2 DETECTED: adding event listeners for page:before-unload and page:change.'); + debugTurbolinks('Detected Turbolinks v2: adding listeners for page:before-unload and page:change.');Also applies to: 43-43, 49-50, 55-55
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
node_package/src/pageLifecycle.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#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.
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#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:
node_package/src/pageLifecycle.ts
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#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:
node_package/src/pageLifecycle.ts
🧬 Code graph analysis (1)
node_package/src/pageLifecycle.ts (1)
node_package/src/turbolinksUtils.ts (5)
turbolinksInstalled(24-26)turbolinksSupported(36-38)turboInstalled(28-30)debugTurbolinks(14-22)turbolinksVersion5(32-34)
⏰ 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: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (3)
node_package/src/pageLifecycle.ts (3)
31-31: Rename reads well and matches scope expansion.Good call on generalizing to “page navigation” vs. Turbolinks-only.
35-38: Confirm no-lib behavior parity.Running page-loaded callbacks immediately when no navigation library is detected seems right; please confirm this matches prior behavior for SSR/hydration so we don’t miss the initial render in those setups.
61-69: Init guard rename/flow looks solid.Prevents double-binding and ensures one-time setup.
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR improves naming clarity in the pageLifecycle.ts file to make the code more self-documenting and better reflect that it handles multiple navigation libraries, not just Turbolinks. The changes address confusing naming that suggested Turbolinks-only functionality when the code actually supports Turbo and multiple Turbolinks versions.
- Renamed functions and variables to be more descriptive and accurate
- Extracted navigation library detection logic into a clearer variable
- Updated debug messages to be more informative about what's being detected
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| node_package/src/pageLifecycle.ts | Renamed functions/variables for clarity and improved debug messages |
| CLAUDE.md | Minor formatting improvements with additional spacing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
CLAUDE.md (3)
37-41: Nit: use “npm” (lowercase) per upstream branding.Minor consistency/branding fix in “Dual Package Structure.”
-This project maintains both a Ruby gem and an NPM package: +This project maintains both a Ruby gem and an npm package: @@ -- **NPM package**: Located in `node_package/src/`, provides client-side React integration +- **npm package**: Located in `node_package/src/`, provides client-side React integration
74-80: Optional: add a brief note aligning contributor guidance with the pageLifecycle naming updates.Helpful for future PRs and keeps this doc in sync with current terminology.
## Important Notes @@ - Generated examples are in `gen-examples/` (ignored by git) +### Naming conventions (navigation libraries) +- Prefer generic “navigation libraries” terminology (Turbo, Turbolinks v5/v2, or none). +- Use updated identifiers in code and docs: `setupPageNavigationListeners`, `isPageLifecycleInitialized`, and `hasNavigationLibrary`. +
10-13: Nit: preferyarn testoveryarn run test(Yarn idiom).Both work, but
yarn testis the conventional form.- - JavaScript tests: `yarn run test` or `rake js_tests` + - JavaScript tests: `yarn test` or `rake js_tests`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
CLAUDE.md(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#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.
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
CLAUDE.md
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#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:
CLAUDE.md
🪛 LanguageTool
CLAUDE.md
[grammar] ~9-~9: There might be a mistake here.
Context: ...l Commands - Install dependencies: bundle && yarn - Run tests: - Ruby tests: `rake run_r...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...ies**: bundle && yarn - Run tests: - Ruby tests: rake run_rspec - JavaScr...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ...nt Setup Commands - Initial setup: bundle && yarn && rake shakapacker_examples:gen_all && rake node_package && rake - Prepare examples: `rake shakapacker_ex...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...ackage && rake- **Prepare examples**:rake shakapacker_examples:gen_all- **Generate node package**:rake node_pack...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...s:gen_all- **Generate node package**:rake node_package- **Run single test example**:rake run_rsp...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...onment Commands - Dummy app tests: rake run_rspec:dummy - Gem-only tests: rake run_rspec:gem -...
(QB_NEW_EN)
[grammar] ~31-~31: There might be a mistake here.
Context: ... run_rspec:dummy- **Gem-only tests**:rake run_rspec:gem- **All tests except examples**:rake all_b...
(QB_NEW_EN)
[grammar] ~47-~47: There might be a mistake here.
Context: ...w helpers for rendering React components - server_rendering_pool.rb: Manages Node.js processes for server-s...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ...e.js processes for server-side rendering - configuration.rb: Global configuration management - **`e...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...n.rb**: Global configuration management - **engine.rb`**: Rails engine integration - **Generator...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...*engine.rb**: Rails engine integration - Generators: Located in `lib/generators...
(QB_NEW_EN)
[grammar] ~55-~55: There might be a mistake here.
Context: ...ntry point for client-side functionality - serverRenderReactComponent.ts: Server-side rendering logic - **`Compo...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ...onent.ts**: Server-side rendering logic - **ComponentRegistry.ts`**: Manages React component registration -...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ...**: Manages React component registration - StoreRegistry.ts: Manages Redux store registration ### ...
(QB_NEW_EN)
[grammar] ~62-~62: There might be a mistake here.
Context: ...- Ruby: Standard gemspec-based build - JavaScript: TypeScript compilation to ...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ...JavaScript**: TypeScript compilation to node_package/lib/ - Testing: Jest for JS, RSpec for Ruby -...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...Testing: Jest for JS, RSpec for Ruby - Linting: ESLint for JS/TS, RuboCop for...
(QB_NEW_EN)
[grammar] ~69-~69: There might be a mistake here.
Context: ...my/` - Rails app for testing integration - Examples: Generated via rake tasks for...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ...sks for different webpack configurations - Rake tasks: Defined in rakelib/ for ...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ...act Server Components support available in Pro version - Generated examples are in...
(QB_NEW_EN)
[grammar] ~85-~85: There might be a mistake here.
Context: ...- /coverage, /tmp, /gen-examples, /node_package/lib - /node_modules, /spec/dummy/node_modules, `/spec/dum...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ...e_modules, /spec/dummy/node_modules, /spec/dummy/tmp-/spec/dummy/app/assets/webpack, /spec/dummy/log`
(QB_NEW_EN)
⏰ 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 (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build
🔇 Additional comments (1)
CLAUDE.md (1)
8-10: Formatting-only changes LGTM; added whitespace improves scanability.The extra blank lines around headings and blocks make this doc easier to skim. No semantic changes detected.
Also applies to: 22-22, 29-30, 37-39, 46-47, 54-55, 61-66, 68-72, 74-80, 82-86, 87-87
* Improve naming clarity in pageLifecycle.ts
The main improvements:
- Rename setupTurbolinksEventListeners() to setupPageNavigationListeners()
to better reflect that it handles multiple navigation libraries, not just Turbolinks
- Rename isEventListenerInitialized to isPageLifecycleInitialized for clarity
- Extract hasNavigationLibrary variable to make the conditional logic clearer
- Update debug messages to be more descriptive ("DETECTED" vs "USING")
- Emphasize that the normal use case is NO navigation library
These changes make the code more self-documenting and easier to understand,
especially given that most users don't use Turbolinks at all.
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
* Fix prettier formatting in CLAUDE.md
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
---------
Co-authored-by: Claude <[email protected]>
Summary
Improves function and variable naming in
pageLifecycle.tsto make the code more self-documenting and easier to understand, especially given that most users don't use Turbolinks at all.Key Changes
setupTurbolinksEventListeners()→setupPageNavigationListeners(): Better reflects that it handles multiple navigation libraries (Turbo, Turbolinks v2/v5), not just TurbolinksisEventListenerInitialized→isPageLifecycleInitialized: More specific about what kind of initialization this trackshasNavigationLibraryvariable: Makes the conditional logic clearer and easier to readWhy This Matters
The original naming was confusing because:
setupTurbolinksEventListeners()suggested it only handled Turbolinks, but it actually handles Turbo and multiple Turbolinks versionsisEventListenerInitializedwere too genericTest Plan
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
New Features
Refactor
Documentation