feat(docs): add 'Edit this page on GitHub' button #4338
Conversation
Zuhair-CS
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughRefactors DocsLayout to centralize edit-link URL construction from edit config and page slug, adds null-guard for post, computes a single Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant DocsLayout
participant EditConfig as editOptions
participant GitHub as GitHub URL
User->>DocsLayout: View page (post available)
DocsLayout->>DocsLayout: guard post != null
DocsLayout->>EditConfig: find target by post.slug
Note right of DocsLayout: parse slug -> last<br/>determine isHrefToFile
DocsLayout->>DocsLayout: build finalUrl (file vs directory, index handling)
User->>DocsLayout: Click "Edit this page" Button
DocsLayout->>GitHub: open(finalUrl, "_blank")
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNone found. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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
Status, Documentation and Community
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
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 ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
components/layout/DocsLayout.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/layout/DocsLayout.tsx (1)
components/buttons/Button.tsx (1)
Button(53-106)
🪛 GitHub Actions: PR testing - if Node project
components/layout/DocsLayout.tsx
[error] 54-54: ESLint: Expected blank line after variable declarations. (newline-after-var)
[error] 55-55: ESLint: Expected blank line before this statement. (padding-line-between-statements)
[error] 66-66: Prettier: Replace "mt-2·ml-2·pt-2·pb-2" with 'mt-2·ml-2·pt-2·pb-2' (prettier/prettier)
[error] 66-66: ESLint: Unexpected usage of doublequote. (jsx-quotes)
[error] 69-69: Prettier: Delete ⏎ (prettier/prettier)
[error] 71-71: ESLint: More than 1 blank line not allowed. (no-multiple-empty-lines)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lighthouse CI
🔇 Additional comments (1)
components/layout/DocsLayout.tsx (1)
41-41: No-op changeThis line change has no functional impact. Nothing to do here.
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4338--asyncapi-website.netlify.app/ |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/layout/DocsLayout.tsx (1)
150-157: Add “Edit this page” button to BlogLayoutThe blog layout currently doesn’t invoke
generateEditLink(post)or render the “Found an error? Have a suggestion?” prompt. To fully meet the objective:
- In
components/layout/BlogLayout.tsx, importgenerateEditLinkfrom the same module as inDocsLayout.tsx.- After the blog post title (e.g. within the header or immediately below), insert:
<p className="font-normal font-sans text-sm text-gray-600 antialiased"> Found an error? Have a suggestion?{generateEditLink(post)} </p>- Mirror the styling and container structure used in
DocsLayout.tsx(around lines 150–157) so the UX is consistent.
♻️ Duplicate comments (1)
components/layout/DocsLayout.tsx (1)
72-72: Delete extraneous blank line flagged by Prettier/ESLintRemoves the extra newline after the function.
Apply this diff:
-
🧹 Nitpick comments (3)
components/layout/DocsLayout.tsx (3)
52-56: Consider appending the “.md” (or index.md) suffix in the directory-join branch as wellIf
target.hrefis a directory (not a file), the constructed URL currently appends only/${last}without an extension, which likely 404s. Consider aligning with the fallback branch’s mapping and append.mdorindex.md.Apply this diff if your
editOptionsdirectory entries expect Markdown files:- } else if (hrefPointsToFile) { + } else if (hrefPointsToFile) { finalUrl = `${target?.href || ''}`; } else { - finalUrl = `${(target?.href || '').replace(/\/+$/, '')}/${last}`; + finalUrl = `${(target?.href || '').replace(/\/+$/, '')}/${post.isIndex ? 'index.md' : `${last}.md`}`; }Before merging, please verify against
config/edit-page-config.jsonthat no directory entry expects a non-Markdown target.
36-36: Redundant null-guard for post (optional)
DocsLayoutalready guardspostat Line 85, and per team learningseditOptionsalways yields a match via a fallback. This early return is likely dead code here.Apply this diff to simplify:
- if (post == null) return null;
43-45: Prefer the most specific match to avoid accidental fallback hits
includes('')is always true; if config order changes,.findmight pick the fallback too early. Consider preferring the longest matchingvalueto make selection robust to ordering.Potential change:
const target = [...editOptions] .sort((a, b) => (b.value?.length || 0) - (a.value?.length || 0)) .find((edit) => post.slug.includes(edit.value));Please verify this against your config to ensure it doesn’t alter intended overrides.
📜 Review details
Configuration used: .coderabbit.yaml
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)
components/layout/DocsLayout.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-editlinks.js:80-93
Timestamp: 2025-01-14T09:23:32.728Z
Learning: In the AsyncAPI website's edit link generation system, the `editOptions` array in `edit-page-config.json` includes a fallback entry with an empty string value (`''`) that matches any URL path, ensuring that `determineEditLink()` function always finds a target and never returns null.
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-edit-links.js:83-88
Timestamp: 2025-01-14T15:12:29.840Z
Learning: In the AsyncAPI website's edit link generation logic, the `editUrls` configuration (config/edit-page-config.json) includes a fallback entry with an empty value that acts as a catch-all matcher. Therefore, `editOptions.find()` will always return a match, making null checks on the result unnecessary and untestable.
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
PR: asyncapi/website#3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
Applied to files:
components/layout/DocsLayout.tsx
🧬 Code graph analysis (1)
components/layout/DocsLayout.tsx (1)
components/buttons/Button.tsx (1)
Button(53-106)
🪛 GitHub Actions: PR testing - if Node project
components/layout/DocsLayout.tsx
[error] 38-38: 'last' is never reassigned. Use 'const' instead.
[error] 46-46: Expected blank line after variable declarations.
[error] 47-47: Expected blank line before this statement.
[error] 49-49: Expected blank line after variable declarations.
[error] 50-50: Expected blank line before this statement.
[error] 52-52: Unexpected if as the only statement in an else block.
[error] 60-60: Fragments should contain more than one child - otherwise, there’s no need for a Fragment at all.
[error] 72-72: Prettier: Delete extraneous newline.
[error] 72-72: No multiple blank lines allowed.
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
🔇 Additional comments (1)
components/layout/DocsLayout.tsx (1)
61-67: LGTM: switched to Button href (proper link semantics + a11y)Good move using
href/targeton the Button; this preserves link semantics (SR, context menu) and adds the ARIA label. Nice.
| let last = post.slug.split('/').pop() || ''; | ||
| const lastListElement = last.split('.'); | ||
| const isHrefToFile = lastListElement.length > 1; | ||
| const EditPage = 'Edit this page on GitHub'; |
There was a problem hiding this comment.
Fix file-target detection (avoid using slug to infer “href points to file”) and resolve CI lint errors in one go
Using the slug’s last segment to decide whether target.href is a file is incorrect (e.g., versioned slugs like 3.0.0 contain dots). This can produce wrong edit URLs. Detect file-ness from target.href instead. Also folds the lonely if into else if and adds required padding newlines. This addresses:
- Incorrect link generation when slug contains dots.
- ESLint:
no-lonely-if,newline-after-var,padding-line-between-statements. - ESLint:
'last' is never reassigned. Use 'const' instead.
Apply this diff:
- let last = post.slug.split('/').pop() || '';
- const lastListElement = last.split('.');
- const isHrefToFile = lastListElement.length > 1;
+ const last = post.slug.split('/').pop() || '';
const EditPage = 'Edit this page on GitHub';
- const target = editOptions.find((edit) => {
- return post.slug.includes(edit.value);
- });
- let finalUrl = '';
+ const target = editOptions.find((edit) => {
+ return post.slug.includes(edit.value);
+ });
+ const hrefPointsToFile = /\.[a-z0-9]+$/i.test(((target?.href || '').split('?')[0] || '').split('#')[0]);
+ let finalUrl = '';
+
if (target?.value === '') {
const base = (target?.href || '').replace(/\/+$/, '');
const slugPath = post.slug.startsWith('/') ? post.slug : `/${post.slug}`;
- finalUrl = post.isIndex ? `${base}${slugPath}/index.md` : `${base}${slugPath}.md`;
- } else {
- if (isHrefToFile) {
- finalUrl = `${target?.href || ''}`;
- } else {
- finalUrl = `${(target?.href || '').replace(/\/+$/, '')}/${last}`;
- }
- }
+
+ finalUrl = post.isIndex ? `${base}${slugPath}/index.md` : `${base}${slugPath}.md`;
+ } else if (hrefPointsToFile) {
+ finalUrl = `${target?.href || ''}`;
+ } else {
+ finalUrl = `${(target?.href || '').replace(/\/+$/, '')}/${last}`;
+ }Also applies to: 43-56
🧰 Tools
🪛 GitHub Actions: PR testing - if Node project
[error] 38-38: 'last' is never reassigned. Use 'const' instead.
🤖 Prompt for AI Agents
In components/layout/DocsLayout.tsx around lines 38-41 (also apply same fixes to
43-56), replace the current slug-based file detection with an href-based check:
stop using the slug's last segment and instead examine target.href (e.g., check
if path or pathname ends with an extension or contains a file-like segment) to
decide isHrefToFile; change mutable let/variables that are never reassigned
(like last) to const; fold the lonely if into an else if to satisfy
no-lonely-if; and add the required blank lines between variable declarations and
subsequent statements to satisfy newline-after-var and
padding-line-between-statements ESLint rules.
| <> | ||
| <Button | ||
| text={EditPage} | ||
| href={finalUrl} | ||
| target='_blank' | ||
| aria-label='Edit this page on GitHub' | ||
| className='mt-2 ml-2 pt-2 pb-2' | ||
| /> | ||
| </> |
There was a problem hiding this comment.
Remove unnecessary Fragment around a single child
The Fragment trips the linter (“Fragments should contain more than one child”). Return the Button directly.
Apply this diff:
- return (
- <>
- <Button
- text={EditPage}
- href={finalUrl}
- target='_blank'
- aria-label='Edit this page on GitHub'
- className='mt-2 ml-2 pt-2 pb-2'
- />
- </>
- );
+ return (
+ <Button
+ text={EditPage}
+ href={finalUrl}
+ target='_blank'
+ aria-label='Edit this page on GitHub'
+ className='mt-2 ml-2 pt-2 pb-2'
+ />
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <> | |
| <Button | |
| text={EditPage} | |
| href={finalUrl} | |
| target='_blank' | |
| aria-label='Edit this page on GitHub' | |
| className='mt-2 ml-2 pt-2 pb-2' | |
| /> | |
| </> | |
| return ( | |
| <Button | |
| text={EditPage} | |
| href={finalUrl} | |
| target='_blank' | |
| aria-label='Edit this page on GitHub' | |
| className='mt-2 ml-2 pt-2 pb-2' | |
| /> | |
| ); |
🧰 Tools
🪛 GitHub Actions: PR testing - if Node project
[error] 60-60: Fragments should contain more than one child - otherwise, there’s no need for a Fragment at all.
🤖 Prompt for AI Agents
In components/layout/DocsLayout.tsx around lines 60 to 68, remove the
unnecessary React Fragment that wraps the single Button child; return the Button
element directly (preserve its props: text={EditPage}, href={finalUrl},
target='_blank', aria-label='Edit this page on GitHub', className='mt-2 ml-2
pt-2 pb-2') and adjust surrounding indentation so the file compiles and the
linter warning is resolved.
2 participants
Description
generateEditLinkhelper function to correctly map.mdx→.md.Related issue(s)
Resolves #4187
Summary by CodeRabbit