Skip to content

Conversation

@ihabadham
Copy link
Collaborator

@ihabadham ihabadham commented Dec 12, 2025

Summary

This PR triggers the link checker CI to observe its behavior more closely. This PR should NOT be merged - it's for investigation only.

Expected vs Actual Behavior

Expected: Link checker should FAIL because master has 21+ broken GitHub URLs (404s)

Actual: We expect CI to pass in ~5 seconds and report "All links are good!" - demonstrating the bug

Investigation

Tracking issue: #2217

We're trying to understand why:

  1. CI completes in ~5 seconds (should take 4+ minutes for 102 files)
  2. CI doesn't detect broken external URLs that return 404
  3. Local npx markdown-link-check correctly finds these errors

What to observe

After CI runs, we'll check:

  • The exact timing of the link checker step
  • Whether any network requests are being made
  • The full logs for any clues about why links aren't being checked

⚠️ Do not merge - close after investigation

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Documentation
    • Minor README formatting updates.

✏️ Tip: You can customize this high-level summary in your review settings.

@claude
Copy link

claude bot commented Dec 12, 2025

Code Review: Investigation PR

Summary

This is an investigation-only PR to observe link checker CI behavior. No code changes are included - this is purely a CI trigger.

Review Notes

✅ Appropriate Approach

  • Correct use of investigation PR: This is the right way to observe CI behavior without polluting commit history
  • Clear documentation: PR description explicitly states this should NOT be merged
  • Good tracking: Properly linked to tracking issue Link checker CI passes despite broken links on master #2217

📋 Observations

PR Structure:

  • Zero files changed (0 additions, 0 deletions)
  • Single commit with clear investigation intent
  • Proper attribution with Co-Authored-By tag

Expected Outcome (per issue #2217):

  • Link checker should FAIL due to 21+ broken GitHub URLs on master
  • But based on recent patterns, likely will pass in ~5 seconds
  • This timing anomaly (5s vs expected 4+ minutes) is the core issue being investigated

🎯 Recommendations

For This PR:

  1. ✅ Do NOT merge (as stated)
  2. 📊 After CI completes, document findings in issue Link checker CI passes despite broken links on master #2217:
    • Exact timing of link checker step
    • Whether any errors were caught
    • Any log clues about network request behavior
  3. 🔍 Check CI logs for:
    • Docker container startup messages
    • Number of files processed
    • Any rate limiting or network warnings

For the Investigation:

  1. Verbose logging: Consider a follow-up test with use-verbose-mode: yes in check-markdown-links.yml:34
  2. Remove retry wrapper: Test without Wandalen/wretry.action wrapper to see if that's interfering
  3. Alternative action: As noted in Link checker CI passes despite broken links on master #2217, the current action has a deprecation notice suggesting Linkspector

Comparison Test Idea:

# Could add a parallel job to compare behaviors
- name: Test without retry wrapper
  uses: tcort/github-action-markdown-link-check@a800ad5f1c35bf61987946fd31c15726a1c9f2ba
  with:
    use-verbose-mode: yes
    config-file: .github/markdown-link-check-config.json

🔍 Code Quality

  • N/A (no code changes)

🐛 Potential Issues

  • N/A (no code changes)

⚡ Performance

  • N/A (no code changes)

🔒 Security

  • N/A (no code changes)

🧪 Test Coverage

  • This PR is a test - it's testing CI infrastructure behavior

Next Steps

  1. Wait for CI to complete on this PR
  2. Document exact timing from GitHub Actions logs
  3. Compare with issue Link checker CI passes despite broken links on master #2217 timeline:
    • Nov 16: 96-99 seconds (caught error)
    • Nov 24+: ~5 seconds (false positive)
  4. Update issue Link checker CI passes despite broken links on master #2217 with findings
  5. Close this PR after gathering data
  6. Consider migration to Linkspector or enhanced debugging

Overall Assessment

Appropriate investigation approach - This is exactly how to debug CI issues without risking master branch health.

⚠️ Remember: Close this PR after data collection. The actual fix should go in a separate PR once root cause is identified.


🤖 Generated with Claude Code

This commit triggers the link checker CI to observe its behavior.
The link checker should fail because master has broken GitHub URLs.

Related: #2217

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@ihabadham ihabadham force-pushed the investigate-link-checker-ci branch from b6e3068 to fe8b6f0 Compare December 12, 2025 23:04
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 12, 2025

Walkthrough

An HTML comment and blank line were added to the README documentation near the tips section. This is a purely documentation-based change with no modifications to code functionality or behavior.

Changes

Cohort / File(s) Change Summary
Documentation
docs/README.md
Added an "Investigation trigger" HTML comment and an extra blank line near the tips section

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem

🐰 A comment slipped in, so quiet and small,
A note for the curious, investigation's call,
No code was changed, no logic bent,
Just a whisper in docs—a gentle event! ✨

Pre-merge checks and finishing touches

✅ 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 accurately describes the PR's primary purpose: investigating link checker CI behavior. It clearly summarizes the main intent without being misleading or vague.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch investigate-link-checker-ci

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8227421 and fe8b6f0.

📒 Files selected for processing (1)
  • docs/README.md (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{rb,js,ts,jsx,tsx,json,yml,yaml,md}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{rb,js,ts,jsx,tsx,json,yml,yaml,md}: ALWAYS run bundle exec rubocop and fix ALL violations before every commit/push
ALWAYS ensure files end with a newline character before committing

Files:

  • docs/README.md
**/*.{js,ts,jsx,tsx,json,css,scss,md}

📄 CodeRabbit inference engine (CLAUDE.md)

ALWAYS let Prettier handle ALL formatting - never manually format code

Files:

  • docs/README.md
🧠 Learnings (6)
📚 Learning: 2025-12-10T09:27:45.105Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.105Z
Learning: Applies to /CHANGELOG.md,/CHANGELOG_PRO.md : Do NOT add changelog entries for: linting, formatting, refactoring, tests, or documentation-only fixes

Applied to files:

  • docs/README.md
📚 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:

  • docs/README.md
📚 Learning: 2025-12-10T09:27:45.105Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.105Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for open-source features, bug fixes, breaking changes, deprecations, and performance improvements using format: `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)`

Applied to files:

  • docs/README.md
📚 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:

  • docs/README.md
📚 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:

  • docs/README.md
📚 Learning: 2025-10-23T17:22:01.074Z
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.

Applied to files:

  • docs/README.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). (1)
  • GitHub Check: claude-review
🔇 Additional comments (1)
docs/README.md (1)

65-66: OK as an investigation-only CI trigger; just ensure trailing newline is preserved.
The added blank line + HTML comment should be safe in Markdown and aligns with the PR’s intent to minimally change docs to trigger CI. Please double-check docs/README.md still ends with a newline (and let Prettier own any formatting).


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 Dec 12, 2025

Code Review: Investigation PR

Summary

This is an investigation-only PR (not meant to be merged) that adds a trivial comment to trigger the link checker CI workflow. The goal is to observe whether the workflow correctly detects broken links.

Code Quality ✅

Change analysis:

  • Adds a single HTML comment <!-- Investigation trigger --> to docs/README.md
  • Minimal, non-invasive change that won't affect documentation rendering
  • Appropriate for its investigation purpose

Investigation Approach ✅

Well-designed test:

  1. Clear hypothesis: CI should fail (broken URLs exist) but is expected to pass (~5 seconds)
  2. Measurable outcome: Can compare timing and results between local tool vs CI
  3. Documented expectations: PR body clearly states what to observe

Observations from CI Results 🔍

The CI workflow for this PR shows:

  • Status: PASSED ✅ (markdown-link-check)
  • Timing: Need to check actual run logs to confirm if it's still ~5 seconds

This confirms the issue described in #2217 - the CI is passing despite known broken links.

Root Cause Analysis (from issue #2217) 🎯

Key findings:

  1. Timeline evidence:

    • Nov 16: CI correctly failed on broken relative link (took 96-99 seconds)
    • Nov 24+: CI passes in ~5 seconds despite 21+ broken GitHub URLs
  2. Broken URLs: All return HTTP 404 after monorepo restructure (PR Restructure monorepo with two top-level product directories #2114)

    • Missing react_on_rails/ prefix in paths
    • Example: /blob/master/lib/generators/... should be /blob/master/react_on_rails/lib/generators/...
  3. 5-second completion time is impossible if network requests are actually being made

    • 102 markdown files × ~1 second per external link check = minutes, not seconds
    • Suggests Docker container isn't making HTTP requests

Recommendations 📋

Immediate actions:

  1. Examine the actual CI run logs from this PR to confirm:

    • Exact timing of the link checker step
    • Whether any network errors/warnings appear
    • Full output from the markdown-link-check action
  2. Test without wretry wrapper:

  3. Consider migration:

    • The tcort/github-action-markdown-link-check action is deprecated
    • Consider switching to Linkspector as recommended

For verification:

# Test locally to confirm broken links exist
npx markdown-link-check docs/api-reference/redux-store-api.md \
  --config .github/markdown-link-check-config.json

# Should report multiple 404 errors for GitHub URLs

Security Concerns ✅

No security issues - this is a documentation-only change.

Performance Considerations ✅

The investigation itself has no performance impact. However, the broken CI workflow is a reliability concern - it's not catching broken links that should fail the build.

Test Coverage ⚠️

This PR achieves its purpose - it triggers the CI to demonstrate the bug. However:

  • The CI itself is the test, and it's failing to do its job
  • Once the root cause is fixed, this exact PR should cause CI to fail (detecting the broken links it's supposed to catch)

Best Practices ✅

Excellent investigation methodology:

  • Clear hypothesis stated upfront
  • Measurable outcomes defined
  • Evidence collected and documented in linked issue
  • PR marked as "Do Not Merge" appropriately

Next Steps

  1. Close this PR after examining CI logs (as intended)
  2. 🔧 Fix the link checker (see recommendations above)
  3. Merge PR Fix outdated paths after monorepo restructuring #2216 to fix the actual broken URLs
  4. Verify fix by re-running link checker on master

Overall Assessment: This is a well-executed diagnostic PR. The investigation reveals a critical CI reliability issue that needs to be addressed. The approach is sound and follows good debugging practices.

🤖 Generated with Claude Code

@ihabadham
Copy link
Collaborator Author

Investigation complete! See #2217 for the root cause analysis.

TL;DR: The markdown-link-check tool crashes silently in the Docker container due to an ESM/CommonJS compatibility error in Node.js 21. The crash message doesn't match the grep pattern used to detect failures, so CI incorrectly reports success.

@ihabadham ihabadham closed this Dec 12, 2025
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.

2 participants