Conversation
- Add LLMs link to navigation header for AI-friendly documentation access - Add anchor-js to show hover links on h2-h4 headings
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds an LLMs link to the manuals navigation header and conditionally loads AnchorJS on manual pages to add hoverable anchor links to H2–H4 headings. Sequence diagram for manual page load with conditional AnchorJSsequenceDiagram
actor User
participant Browser
participant Server
participant AnchorJSCDN
User->>Browser: Request_manual_page
Browser->>Server: GET_manual_page
Server-->>Browser: HTML_with_manual_header_and_footer
Browser->>Browser: Parse_HTML
alt Page_category_is_Manual
Browser->>AnchorJSCDN: GET_anchor_min_js
AnchorJSCDN-->>Browser: anchor_min_js
Browser->>Browser: Execute_anchors_add_h2_h3_h4
else Page_category_is_not_Manual
Browser->>Browser: Skip_loading_AnchorJS
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughTwo Jekyll template includes receive additions: the footer conditionally loads anchor-js for Manual category pages to generate headings anchors, and the header adds a new "LLMs" navigation link. Both are non-breaking, isolated enhancements. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider scoping
anchor-jsto the article container (e.g.anchors.add('#article h2, #article h3, #article h4');) so it doesn’t unexpectedly add anchors to headings elsewhere on the page. - If
llms-full.txtis intended to behave like other main navigation destinations, think about whether it should get active state handling and/or any specific styling class so it’s visually consistent with the rest of the nav.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider scoping `anchor-js` to the article container (e.g. `anchors.add('#article h2, #article h3, #article h4');`) so it doesn’t unexpectedly add anchors to headings elsewhere on the page.
- If `llms-full.txt` is intended to behave like other main navigation destinations, think about whether it should get active state handling and/or any specific styling class so it’s visually consistent with the rest of the nav.
## Individual Comments
### Comment 1
<location> `_includes/manuals/1.0/footer.html:18-19` </location>
<code_context>
<script src="//cdnjs.cloudflare.com/ajax/libs/popper.js/1.14.3/umd/popper.min.js" integrity="sha384-ZMP7rVo3mIykV+2+9J3UJ46jBk0WLaUAdn689aCwoqbBJiSnjAK/l8WvCWPIPm49" crossorigin="anonymous"></script>
<script src="//stackpath.bootstrapcdn.com/bootstrap/4.1.3/js/bootstrap.min.js" integrity="sha384-ChfqqxuZUCnJSK3+MXmPNIyE6ZbWh2IMqE241rYiqJxyMiZ6OW/JmZQ5stwEULTy" crossorigin="anonymous"></script>
+{% if page.category == 'Manual' %}
+<script src="https://cdn.jsdelivr.net/npm/anchor-js/anchor.min.js"></script>
+<script>anchors.add('h2, h3, h4');</script>
+{% endif %}
<script src="/js/generate-table-of-contents/index.min.js"></script>
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider adding SRI attributes to the new CDN-loaded script for consistency and security.
The existing third-party scripts here all specify `integrity` and `crossorigin`, but the new `anchor-js` script does not. Please add the appropriate SRI hash and `crossorigin` value (if supported by the CDN) to keep our security controls consistent.
Suggested implementation:
```
{% if page.category == 'Manual' %}
<script src="https://cdn.jsdelivr.net/npm/[email protected]/anchor.min.js"
integrity="<!-- TODO: insert SRI hash for [email protected]/anchor.min.js -->"
crossorigin="anonymous"></script>
<script>anchors.add('h2, h3, h4');</script>
{% endif %}
```
1. Compute the correct SRI hash for `https://cdn.jsdelivr.net/npm/[email protected]/anchor.min.js` (e.g., using `openssl dgst -sha384 -binary | openssl base64 -A`) and replace the `<!-- TODO: ... -->` placeholder with the real `sha384-...` value.
2. If you prefer to track a different specific version of `anchor-js`, adjust the `@4.3.1` segment and compute the SRI hash for that exact URL instead.
</issue_to_address>
### Comment 2
<location> `_includes/manuals/1.0/footer.html:17-20` </location>
<code_context>
<script src="//stackpath.bootstrapcdn.com/bootstrap/4.1.3/js/bootstrap.min.js" integrity="sha384-ChfqqxuZUCnJSK3+MXmPNIyE6ZbWh2IMqE241rYiqJxyMiZ6OW/JmZQ5stwEULTy" crossorigin="anonymous"></script>
+{% if page.category == 'Manual' %}
+<script src="https://cdn.jsdelivr.net/npm/anchor-js/anchor.min.js"></script>
+<script>anchors.add('h2, h3, h4');</script>
+{% endif %}
<script src="/js/generate-table-of-contents/index.min.js"></script>
</code_context>
<issue_to_address>
**suggestion:** Limit anchor-js scope to the article container to avoid affecting non-content headings.
`anchors.add('h2, h3, h4');` applies to all headings on the page, which can include nav/sidebar/footer content. Since the table of contents is already scoped to `#article`, consider matching that scope (e.g. `anchors.add('#article h2, #article h3, #article h4');`) so anchors are only added to main content headings.
```suggestion
{% if page.category == 'Manual' %}
<script src="https://cdn.jsdelivr.net/npm/anchor-js/anchor.min.js"></script>
<script>anchors.add('#article h2, #article h3, #article h4');</script>
{% endif %}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+18
to
+19
| <script src="https://cdn.jsdelivr.net/npm/anchor-js/anchor.min.js"></script> | ||
| <script>anchors.add('h2, h3, h4');</script> |
Contributor
There was a problem hiding this comment.
🚨 suggestion (security): Consider adding SRI attributes to the new CDN-loaded script for consistency and security.
The existing third-party scripts here all specify integrity and crossorigin, but the new anchor-js script does not. Please add the appropriate SRI hash and crossorigin value (if supported by the CDN) to keep our security controls consistent.
Suggested implementation:
{% if page.category == 'Manual' %}
<script src="https://cdn.jsdelivr.net/npm/[email protected]/anchor.min.js"
integrity="<!-- TODO: insert SRI hash for [email protected]/anchor.min.js -->"
crossorigin="anonymous"></script>
<script>anchors.add('h2, h3, h4');</script>
{% endif %}
- Compute the correct SRI hash for
https://cdn.jsdelivr.net/npm/[email protected]/anchor.min.js(e.g., usingopenssl dgst -sha384 -binary | openssl base64 -A) and replace the<!-- TODO: ... -->placeholder with the realsha384-...value. - If you prefer to track a different specific version of
anchor-js, adjust the@4.3.1segment and compute the SRI hash for that exact URL instead.
Comment on lines
+17
to
+20
| {% if page.category == 'Manual' %} | ||
| <script src="https://cdn.jsdelivr.net/npm/anchor-js/anchor.min.js"></script> | ||
| <script>anchors.add('h2, h3, h4');</script> | ||
| {% endif %} |
Contributor
There was a problem hiding this comment.
suggestion: Limit anchor-js scope to the article container to avoid affecting non-content headings.
anchors.add('h2, h3, h4'); applies to all headings on the page, which can include nav/sidebar/footer content. Since the table of contents is already scoped to #article, consider matching that scope (e.g. anchors.add('#article h2, #article h3, #article h4');) so anchors are only added to main content headings.
Suggested change
| {% if page.category == 'Manual' %} | |
| <script src="https://cdn.jsdelivr.net/npm/anchor-js/anchor.min.js"></script> | |
| <script>anchors.add('h2, h3, h4');</script> | |
| {% endif %} | |
| {% if page.category == 'Manual' %} | |
| <script src="https://cdn.jsdelivr.net/npm/anchor-js/anchor.min.js"></script> | |
| <script>anchors.add('#article h2, #article h3, #article h4');</script> | |
| {% endif %} |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@sourcery-ai Added SRI attributes and scoped anchor-js to #article container. |
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
Summary by Sourcery
Add an LLMs navigation link to the manuals header and enable anchored heading links in manual pages.
New Features:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.