Skip to content

Conversation

@ihabadham
Copy link
Contributor

@ihabadham ihabadham commented Sep 10, 2025

  • Fix formatting in CHANGELOG.md
  • Fix formatting in CODING_AGENTS.md
  • Fix formatting in docs files
  • Fix formatting in CSS module test fixtures

These formatting issues were introduced in the merged PR #1769 and are causing CI failures.

Summary

Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.

Pull Request checklist

Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by ~.

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

Add the CHANGELOG entry at the top of the file.

Other Information

Remove this paragraph and mention any other important and relevant information such as benchmarks.


This change is Reviewable

Summary by CodeRabbit

  • Documentation

    • Updated react_component example to use a props wrapper in a two-argument form.
    • Revised hydration guidance: no need to defer scripts; ReactOnRails.reactOnRailsPageLoaded is async; added Redux force_load notes and per-store overrides; clarified Turbolinks usage.
    • Refined upgrading guide formatting (removed “Breaking Configuration Changes” heading) without changing instructions.
    • Improved readability with spacing tweaks across multiple guides and Getting Started.
  • Style

    • Whitespace-only changes, including added blank lines and trailing newlines; no functional impact.

- Fix formatting in CHANGELOG.md
- Fix formatting in CODING_AGENTS.md
- Fix formatting in docs files
- Fix formatting in CSS module test fixtures

These formatting issues were introduced in the merged PR #1769
and are causing CI failures.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 10, 2025

Walkthrough

This PR primarily updates documentation formatting, adjusts a migration example for react_component props structure, expands v15 release notes around hydration behavior, and adds trailing newlines to two fixture files. No code logic or public API changes are introduced.

Changes

Cohort / File(s) Summary of Changes
Docs: formatting-only
CHANGELOG.md, CODING_AGENTS.md, docs/getting-started.md, docs/guides/streaming-server-rendering.md, docs/guides/upgrading-react-on-rails.md
Whitespace/heading tweaks (blank lines added; heading removal in upgrading doc). No semantic/content changes to guidance or APIs.
Docs: migration example update
docs/additional-details/migrating-from-react-rails.md
Updated react_component example to use two-argument form with props nested under a props key alongside prerender. Content otherwise unchanged.
Docs: release notes v15
docs/release-notes/15.0.0.md
Clarifies hydration behavior: no need to defer scripts; ReactOnRails.reactOnRailsPageLoaded() is async and should be awaited; introduces Redux store force_load with config default and per-store override; Turbolinks listener guidance.
Fixtures: EOF newlines
spec/dummy/spec/fixtures/.../ComponentWithCSSModule/ror_components/ComponentWithCSSModule.jsx, spec/dummy/spec/fixtures/.../ComponentWithCSSModule/ror_components/ComponentWithCSSModule.module.css
Added trailing newline at EOF; no functional or content changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor U as User
  participant B as Browser
  participant R as ReactOnRails
  participant S as Redux Store
  participant C as React Components

  U->>B: Navigate to page
  B->>R: Load page scripts (no defer required)
  rect rgba(230,245,255,0.5)
    note over B,R: Streaming HTML begins
    R-->>B: Initial HTML stream
    R->>R: reactOnRailsPageLoaded() (async)
    R-->>B: Promise pending
  end

  alt force_load true (default)
    R->>S: Initialize stores
  else force_load false (per-store override)
    R->>S: Defer store init until needed
  end

  R->>C: Hydrate components
  R-->>B: Promise resolves when hydration complete
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • Judahmeek
  • alexeyr-ci2

Pre-merge checks (3 passed)

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “Fix prettier formatting issues from PR #1769” accurately identifies that the pull request primarily resolves styling and whitespace discrepancies introduced by the earlier PR, while remaining concise and clearly signaling the nature of the fix.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

Poem

Thump-thump, I hop through docs so bright,
Newlines sprinkled, spacing just right.
Props tucked snug in a cozy nest,
Hydration awaits—async at best.
CSS files purr, a newline’s grace,
Release notes hum at a gentler pace.
Carrot-merge! Onward, ship this case. 🥕✨

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.

Please see the documentation for more information.

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 fix/prettier-formatting-from-pr-1769

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.

@claude
Copy link

claude bot commented Sep 10, 2025

Code Review

Summary

This PR addresses formatting issues from PR #1769 that were causing CI failures. The changes are purely cosmetic and focus on fixing prettier/linting violations.

✅ Code Quality & Best Practices

  • Good: All changes are formatting-only, maintaining consistency with project standards
  • Good: Proper markdown formatting with blank lines after numbered list items
  • Good: Fixed missing newlines at end of files in test fixtures
  • Good: Consistent indentation and spacing in documentation

✅ Potential Issues

  • No functional bugs: All changes are cosmetic formatting fixes
  • Safe changes: No logic or behavior modifications
  • Low risk: Formatting changes have minimal impact on functionality

✅ Performance Considerations

  • No impact: Formatting changes don't affect runtime performance
  • Documentation only: Most changes are in markdown files that don't affect application performance

✅ Security Concerns

  • No security impact: Changes are limited to formatting and documentation
  • Safe: No code logic or data handling modifications

✅ Test Coverage

  • No new tests needed: Formatting changes don't require additional test coverage
  • Existing tests sufficient: The changes don't modify any testable functionality
  • Note: The PR checklist mentions adding tests, but this is not applicable for formatting fixes

Recommendations

  1. Complete PR checklist: Update the checklist to reflect that tests/documentation updates aren't needed for formatting fixes
  2. Consider automation: To prevent similar issues in the future, consider adding a pre-commit hook or GitHub Action for prettier/linting
  3. Ready to merge: These are necessary fixes for CI failures and should be merged promptly

Overall Assessment

This is a straightforward formatting fix that addresses CI failures. The changes are minimal, safe, and necessary for maintaining code quality standards.

Recommendation: ✅ Approve and merge

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
docs/release-notes/15.0.0.md (2)

61-63: Tighten wording and branding for clarity

Minor copy edits to improve readability and use the product name vs. package name.

-  - The previous need for deferring scripts to prevent race conditions has been eliminated due to improved hydration handling. Making scripts not defer is critical to execute the hydration scripts early before the page is fully loaded.
+  - The prior need to defer scripts to prevent race conditions has been eliminated by improved hydration handling. Do not defer generated component packs; this ensures hydration scripts execute early, before the page is fully loaded.
-  - The `force_load` configuration makes `react-on-rails` hydrate components immediately as soon as their server-rendered HTML reaches the client, without waiting for the full page load.
+  - The `force_load` configuration makes React on Rails hydrate components immediately as soon as their server-rendered HTML reaches the client, without waiting for the full page load.

68-82: Note: call site must be in an async context

Add a one-liner so readers don’t trip on environments without top‑level await.

   - If you manually call this function to ensure components are hydrated (e.g., with async script loading), you must now await the promise it returns:
+  - Ensure this call runs in an async context (or wrap in an async IIFE) if your environment doesn’t support top‑level `await`.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 52a1855 and f54e964.

📒 Files selected for processing (9)
  • CHANGELOG.md (2 hunks)
  • CODING_AGENTS.md (1 hunks)
  • docs/additional-details/migrating-from-react-rails.md (1 hunks)
  • docs/getting-started.md (1 hunks)
  • docs/guides/streaming-server-rendering.md (2 hunks)
  • docs/guides/upgrading-react-on-rails.md (4 hunks)
  • docs/release-notes/15.0.0.md (2 hunks)
  • spec/dummy/spec/fixtures/automated_packs_generation/components/ComponentWithCSSModule/ror_components/ComponentWithCSSModule.jsx (1 hunks)
  • spec/dummy/spec/fixtures/automated_packs_generation/components/ComponentWithCSSModule/ror_components/ComponentWithCSSModule.module.css (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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:

  • docs/additional-details/migrating-from-react-rails.md
  • docs/guides/upgrading-react-on-rails.md
📚 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:

  • docs/release-notes/15.0.0.md
⏰ 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: rspec-package-tests (newest)
  • GitHub Check: rspec-package-tests (oldest)
  • GitHub Check: build
🔇 Additional comments (9)
docs/getting-started.md (1)

41-41: Prettier spacing after heading — looks good

Blank line after “3. Start the app:” fixes Markdown list rendering.

CODING_AGENTS.md (1)

262-262: Consistent Markdown spacing

Inserted blank lines after numbered subheadings improve formatting and satisfy Prettier.

Also applies to: 268-268, 273-273

spec/dummy/spec/fixtures/automated_packs_generation/components/ComponentWithCSSModule/ror_components/ComponentWithCSSModule.module.css (1)

9-9: Add trailing newline

EOF newline aligns with linters/CI expectations.

spec/dummy/spec/fixtures/automated_packs_generation/components/ComponentWithCSSModule/ror_components/ComponentWithCSSModule.jsx (1)

12-12: Add trailing newline

Good fix for prettier/CI.

docs/guides/streaming-server-rendering.md (1)

122-122: Prettier-compliant blank lines

Spacing tweaks improve list and section rendering; no content changes.

Also applies to: 168-168, 174-174

CHANGELOG.md (1)

445-445: Whitespace-only cleanup

Extra blank lines improve readability and satisfy formatting checks.

Also applies to: 1163-1163, 1169-1169

docs/additional-details/migrating-from-react-rails.md (1)

6-6: Correct helper signature example

Switch to two-arg form with props: key is accurate and consistent with current API.

Also applies to: 14-14, 18-18, 27-30

docs/guides/upgrading-react-on-rails.md (1)

240-240: Formatting-only adjustments

Removal of redundant subheading and spacing changes improve structure; content unchanged.

Also applies to: 253-253, 266-266, 296-296

docs/release-notes/15.0.0.md (1)

60-60: LGTM: formatting fix unblocks Prettier/Markdown nesting

The inserted blank line correctly separates paragraphs from the nested list and should satisfy the formatter.

@claude
Copy link

claude bot commented Sep 10, 2025

Code Review - PR #1774: Fix prettier formatting issues from PR #1769

Overall Assessment ✅

This is a clean-up PR that addresses formatting inconsistencies introduced in PR #1769. The changes are primarily cosmetic and focus on improving code consistency and readability.

Code Quality and Best Practices ✅

Strengths:

  • All changes are formatting-only with no functional impact
  • Consistent application of formatting rules across documentation and test files
  • Proper trailing newlines added to JavaScript/CSS files
  • Improved markdown formatting with appropriate spacing

Areas of Excellence:

  • Clean separation of list items in documentation with proper blank lines
  • Consistent indentation and spacing in markdown files
  • Proper file endings (trailing newlines) in source files

Potential Issues - None Identified ✅

No functional bugs or issues detected. All changes are cosmetic formatting improvements.

Performance Considerations ✅

No performance impact - these are documentation and formatting changes only.

Security Concerns ✅

No security implications as this is purely formatting changes.

Test Coverage 📝

Current Status: The PR checklist shows unchecked items for tests and documentation updates.

Recommendation: For a formatting-only PR like this:

  • Tests are not needed since no functional changes were made
  • Documentation updates are not required as the content remains the same
  • Consider checking these items and wrapping with ~ to indicate they are not applicable

Specific Changes Reviewed:

  1. CHANGELOG.md - Added proper spacing around numbered lists and bullet points
  2. CODING_AGENTS.md - Improved list formatting with blank line separators
  3. Documentation files - Enhanced readability with consistent spacing
  4. Test fixtures - Added proper trailing newlines to JavaScript and CSS files

Recommendations:

  1. Approve and merge - This PR successfully fixes the formatting issues
  2. 📝 Update PR checklist - Mark non-applicable items as completed with ~ wrapping
  3. 🔄 Consider automated formatting - This type of issue could be prevented with:
    • Pre-commit hooks running prettier/eslint
    • Automated formatting checks in CI

Conclusion

This is a well-executed formatting fix that maintains code quality standards. The changes align with the repository's formatting conventions and successfully address the CI failures mentioned in the PR description.

Status: APPROVED

@justin808
Copy link
Member

update docs on how to lint, prettier

@justin808 justin808 merged commit b97c854 into master Sep 10, 2025
13 checks passed
@justin808 justin808 deleted the fix/prettier-formatting-from-pr-1769 branch September 10, 2025 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants