feat: resolve relative markdown links to internal docs routes - #31
Merged
Conversation
Convert relative .md links (e.g. installation.md, ../README.md) found
in documentation files to internal /docs/{page} URLs, falling back to
GitHub URLs only for external or unresolvable links.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughDocumentService now supplies the source file path to MarkdownService when rendering. MarkdownService gained source-path-aware link resolution (including Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Services/MarkdownService.php (1)
288-296: The page validation regex rejects subdirectory paths and certain characters.The regex
[a-z0-9-]+at line 291 rejects:
- Uppercase characters (e.g.,
README.md)- Underscores (e.g.,
getting_started.md)- Subdirectory paths (e.g.,
recipes/laravel.md→ page would berecipes/laravel)This causes the
operations/runbooks.mdlink in the test fixture to fall through to GitHub URL, which appears intentional based on the test assertions. However, if the intent is to support nested docs in the future, this would need adjustment.📝 Consider documenting this intentional constraint
$page = preg_replace('/\.md$/', '', $relativeTargetPath); + // Only resolve flat docs pages (no subdirectories, lowercase alphanumeric + hyphens) + // Nested paths like operations/runbooks.md fall through to GitHub URLs if ($page === null || $page === '' || preg_match('/^[a-z0-9-]+$/', $page) !== 1) { return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/MarkdownService.php` around lines 288 - 296, The current validation for $page (computed from $resolvedTargetPath and $docsPrefix) uses preg_match('/^[a-z0-9-]+$/') which rejects uppercase letters, underscores and subdirectory paths; update the validation in the method that computes $relativeTargetPath/$page to use a more permissive pattern such as allowing letters (both cases), digits, underscores, hyphens and forward slashes (e.g. /^[A-Za-z0-9_\/-]+$/) so filenames like README.md, getting_started.md and nested paths like recipes/laravel.md pass, and ensure preg_replace that strips '.md' remains applied to $relativeTargetPath before the preg_match check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Services/DocumentService.php`:
- Line 84: The markdown-to-HTML call in DocumentService (the line calling
$this->markdown->toHtml in DocumentService.php) is passing $titleFallback as the
$currentSection which causes relative links to be resolved as if they live under
a subdirectory; change the call to pass null for $currentSection (same as
README) or pass the actual $sourceFilePath used for GitHub resolution so links
like [Installation](installation.md) resolve to docs/installation.md instead of
docs/<page>/installation.md; update the invocation in the method that constructs
$html and ensure any tests or callers expecting the old behavior are adjusted
accordingly.
---
Nitpick comments:
In `@app/Services/MarkdownService.php`:
- Around line 288-296: The current validation for $page (computed from
$resolvedTargetPath and $docsPrefix) uses preg_match('/^[a-z0-9-]+$/') which
rejects uppercase letters, underscores and subdirectory paths; update the
validation in the method that computes $relativeTargetPath/$page to use a more
permissive pattern such as allowing letters (both cases), digits, underscores,
hyphens and forward slashes (e.g. /^[A-Za-z0-9_\/-]+$/) so filenames like
README.md, getting_started.md and nested paths like recipes/laravel.md pass, and
ensure preg_replace that strips '.md' remains applied to $relativeTargetPath
before the preg_match check.
| } | ||
|
|
||
| $html = $this->markdown->toHtml($content); | ||
| $html = $this->markdown->toHtml($content, $documentIdentity === 'readme' ? null : $titleFallback, $filePath); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how currentSection is used in resolveGitHubUrl
echo "=== resolveGitHubUrl implementation ==="
rg -nA 20 'private function resolveGitHubUrl' app/Services/MarkdownService.php
echo -e "\n=== loadFromPath calls and titleFallback usage ==="
rg -nB 2 -A 5 'toHtml' app/Services/DocumentService.php | head -50
echo -e "\n=== MarkdownService toHtml signature ==="
rg -nA 5 'public function toHtml' app/Services/MarkdownService.phpRepository: loadinglucian/deployerphp.com
Length of output: 1633
🏁 Script executed:
#!/bin/bash
# Get complete resolveGitHubUrl function
echo "=== Complete resolveGitHubUrl implementation ==="
rg -nA 40 'private function resolveGitHubUrl' app/Services/MarkdownService.php
echo -e "\n=== How titleFallback is set in DocumentService ==="
rg -nB 10 'titleFallback' app/Services/DocumentService.php | grep -A 10 -B 10 'titleFallback ='
echo -e "\n=== toHtml method body to see how currentSection is used ==="
rg -nA 30 'public function toHtml' app/Services/MarkdownService.phpRepository: loadinglucian/deployerphp.com
Length of output: 2949
🏁 Script executed:
#!/bin/bash
# Check the docs directory structure
echo "=== Docs directory structure ==="
find docs -type f -name "*.md" 2>/dev/null | head -30
echo -e "\n=== How titleFallback is populated in DocumentService ==="
rg -nB 20 'toHtml' app/Services/DocumentService.php | grep -A 15 -B 15 'titleFallback'
echo -e "\n=== Load method to see how titleFallback comes from file path ==="
rg -nA 10 'public function load' app/Services/DocumentService.php | head -30Repository: loadinglucian/deployerphp.com
Length of output: 1429
🏁 Script executed:
#!/bin/bash
# Check the loadFromPath signature and parameters
echo "=== DocumentService loadFromPath signature ==="
rg -nA 20 'private function loadFromPath' app/Services/DocumentService.php
echo -e "\n=== Check if docs directory exists and structure ==="
ls -la docs/ 2>/dev/null || echo "docs/ not found, checking other locations"
find . -maxdepth 2 -type d -name docs 2>/dev/null
echo -e "\n=== Check if there are test files or fixtures showing docs structure ==="
find . -name "*.md" -type f 2>/dev/null | head -20
echo -e "\n=== Check config for docs.github.dir ==="
rg -n 'docs\.github' config/Repository: loadinglucian/deployerphp.com
Length of output: 1253
🏁 Script executed:
#!/bin/bash
# Check the test fixture docs structure
echo "=== Test fixture docs structure ==="
tree tests/Fixtures/docs/ 2>/dev/null || find tests/Fixtures/docs/ -type f
echo -e "\n=== Check test files that use these docs ==="
rg -l 'DocumentService' tests/ | head -5
echo -e "\n=== Look at a test file to understand expected behavior ==="
find tests/ -name "*Document*Test*" -type f -exec cat {} \; | head -100
echo -e "\n=== Check what config value is used for docs.github.dir ==="
rg -n "docs\.github\.dir" . 2>/dev/nullRepository: loadinglucian/deployerphp.com
Length of output: 3785
🏁 Script executed:
#!/bin/bash
# Check MarkdownServiceTest for GitHub URL tests
echo "=== MarkdownServiceTest GitHub URL tests ==="
rg -nA 20 'github' tests/Unit/MarkdownServiceTest.php -i
echo -e "\n=== Check if there are tests for resolveGitHubUrl specifically ==="
rg -nA 15 'resolveGitHubUrl\|currentSection' tests/Unit/MarkdownServiceTest.php
echo -e "\n=== Look at the actual test fixtures to see how links are expected to work ==="
cat tests/Fixtures/docs/docs/link-behavior.mdRepository: loadinglucian/deployerphp.com
Length of output: 2701
🏁 Script executed:
#!/bin/bash
# Check if there are tests for actual page loading with links
echo "=== Check DocumentServiceTest for link behavior ==="
cat tests/Unit/DocumentServiceTest.php
echo -e "\n=== Check if docs structure is hierarchical or flat in fixture ==="
rg -n "relative\|link" tests/Fixtures/docs/docs/*.md -i -A 2 -B 2Repository: loadinglucian/deployerphp.com
Length of output: 3042
🏁 Script executed:
#!/bin/bash
# Read the actual fixture docs
echo "=== installation.md ==="
cat tests/Fixtures/docs/docs/installation.md
echo -e "\n=== documentation.md ==="
cat tests/Fixtures/docs/docs/documentation.md
echo -e "\n=== link-behavior.md ==="
cat tests/Fixtures/docs/docs/link-behavior.mdRepository: loadinglucian/deployerphp.com
Length of output: 752
🏁 Script executed:
#!/bin/bash
# Test what happens when documentation.md (flat structure) contains relative links
echo "=== Test the actual behavior with flat docs ==="
cat > /tmp/test_url.php << 'EOF'
<?php
// Simulate the resolveGitHubUrl logic
function testResolveGitHubUrl(string $href, ?string $currentSection): string {
$docsDir = 'docs';
$baseUrl = 'https://github.com/acme/repo/blob/main';
// Handle .md links without slashes
if (str_ends_with($href, '.md')) {
if ($currentSection !== null && ! str_contains($href, '/')) {
return "{$baseUrl}/{$docsDir}/{$currentSection}/{$href}";
}
return "{$baseUrl}/{$docsDir}/{$href}";
}
// Other relative paths
if ($currentSection !== null && ! str_contains($href, '/')) {
return "{$baseUrl}/{$docsDir}/{$currentSection}/{$href}.md";
}
return "{$baseUrl}/{$docsDir}/{$href}.md";
}
// Test case: documentation.md page with relative links to installation.md
echo "When documentation page contains '[Installation](installation.md)':\n";
echo "Generated URL: " . testResolveGitHubUrl('installation.md', 'documentation') . "\n";
echo "Expected URL: https://github.com/acme/repo/blob/main/docs/installation.md\n\n";
// Test case: link with slash (should work correctly)
echo "When page contains '[Operations](operations/runbooks.md)':\n";
echo "Generated URL: " . testResolveGitHubUrl('operations/runbooks.md', 'documentation') . "\n";
echo "Expected URL: https://github.com/acme/repo/blob/main/docs/operations/runbooks.md\n";
EOF
php /tmp/test_url.phpRepository: loadinglucian/deployerphp.com
Length of output: 576
Fix incorrect GitHub URL generation for relative links in flat docs structure.
Passing $titleFallback (the page slug) as $currentSection causes relative links without directory separators to generate incorrect GitHub URLs. For example, when documentation.md contains [Installation](installation.md), it generates https://github.com/repo/blob/main/docs/documentation/installation.md instead of https://github.com/repo/blob/main/docs/installation.md.
In a flat docs structure where pages like installation.md and documentation.md live directly in the docs directory, they should not have a "current section" prepended. The null value passed for README is correct; regular pages should use the same approach or $sourceFilePath should be used instead for GitHub URL resolution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Services/DocumentService.php` at line 84, The markdown-to-HTML call in
DocumentService (the line calling $this->markdown->toHtml in
DocumentService.php) is passing $titleFallback as the $currentSection which
causes relative links to be resolved as if they live under a subdirectory;
change the call to pass null for $currentSection (same as README) or pass the
actual $sourceFilePath used for GitHub resolution so links like
[Installation](installation.md) resolve to docs/installation.md instead of
docs/<page>/installation.md; update the invocation in the method that constructs
$html and ensure any tests or callers expecting the old behavior are adjusted
accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
resolveRelativeDocsUrltoMarkdownServiceto convert relative.mdlinks (e.g.installation.md,../README.md) into internal/docs/{page}routes$sourceFilePaththroughtoHtml→transformLinks→transformLinkso the resolver can anchor relative paths against the source file's directoryTest plan
DocsSidebarNavigationTestcovers: README relative link → home route, sibling.mdlink → internal doc route, subdirectory link → GitHub URLphp artisan test --compact tests/Feature/DocsSidebarNavigationTest.phpto verify