feat(accessibility): implement skip to main content functionality #4624
Conversation
Devnil434
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg and
sambhavgupta0705
as code owners
asyncapi-bot
requested review from
CBID2,
ashmitjsg,
bandantonio and
vishvamsinh28
WalkthroughAdds an accessible skip-to-main-content link and styles/JS, makes TOC item Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant SkipLink as "Skip Link (anchor)"
participant Main as "Main Content (`#main-content`)"
User->>Browser: Load page and press Tab
Browser->>SkipLink: Focus lands on hidden anchor
Note right of SkipLink: CSS :focus reveals link
User->>SkipLink: Activate (Enter)
SkipLink->>Main: Browser jumps to `#main-content`
Browser->>Main: Focus moves to main content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ 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). (2)
🔇 Additional comments (1)
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 |
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Devnil434
changed the title
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4624--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
components/layout/DocsLayout.tsx (1)
139-253: Docs main wrapper now provides a skip-link target; addtabIndexfor reliable focus.Using
<main className="flex flex-row" id="main-content">is the right place for the skip-link anchor to land. To ensure that activating the skip link also moves keyboard focus into the content consistently across browsers, consider addingtabIndex={-1}to thismainelement:- <main className="flex flex-row" id="main-content"> + <main className="flex flex-row" id="main-content" tabIndex={-1}>This keeps the element out of the normal tab order but allows focus when reached via the skip link or programmatically.
components/layout/GenericLayout.tsx (1)
25-55: Generic layout integrates well with skip link; ensure main is focusable.The
main id="main-content"wrapper gives generic pages a proper target for the global skip link, which is great. For consistent keyboard behavior, I recommend addingtabIndex={-1}so activating the skip link moves focus into the main content reliably:- <main id="main-content" data-testid="Generic-main"> + <main id="main-content" data-testid="Generic-main" tabIndex={-1}>Everything else in this refactor (error handling, test IDs, banner placement) looks good.
🧹 Nitpick comments (6)
pages/[lang]/index.tsx (1)
1-30: Consider completing the absolute import refactor.Lines 1-5 use absolute imports (
@/types/...,@/utils/...), but lines 7-30 still use relative imports (../../components/...). For consistency and maintainability, consider converting all imports to use the absolute@/alias pattern.If you'd like, apply this pattern to the remaining imports:
-import Button from '../../components/buttons/Button'; -import GoogleCalendarButton from '../../components/buttons/GoogleCalendarButton'; +import Button from '@/components/buttons/Button'; +import GoogleCalendarButton from '@/components/buttons/GoogleCalendarButton';...and so forth for all component imports.
fix-line-endings.js (1)
1-16: Consider removing this utility script after use.This appears to be an ad-hoc utility script for normalizing line endings in specific files. If this was a one-time fix, consider removing it after running to avoid confusion. If this needs to be reusable, consider:
- Adding it as an npm script in package.json
- Making it more generic (accepting file paths as arguments)
- Documenting when/why it should be used
Additionally, there's a related
fix-line-endings.cjsin the PR that uses a different approach (recursive MDX discovery). Consider consolidating these if both serve the same purpose.components/layout/Layout.tsx (1)
62-66: Remove unnecessary type assertion.The explicit
as React.JSX.Elementtype assertion on line 66 is unnecessary since JSX elements are already correctly typed. TypeScript will infer the correct return type.Apply this diff:
- return ( - <div id='main-content'> - {children} - </div> - ) as React.JSX.Element; + return ( + <div id='main-content'> + {children} + </div> + );fix-line-endings.cjs (1)
5-35: MDX normalization script looks good; consider minor robustness tweaks.Logic and sync I/O usage are appropriate for a one-off tooling script. Two small optional improvements:
- Make the root directory configurable (or at least document that only
pages/**/*.mdxare processed) in case MDX lives elsewhere.- Skip
writeFileSyncwhenfixedContent === contentto avoid unnecessary rewrites and no-op git diffs.pages/_app.tsx (1)
21-52: Skip-link visibility logic works but could be simplified and made more robust.The keyboard-intent detection via
Taband hiding on click is sound, but directly mutatingstyle.topon the link can conflict with the CSS:focusrule and is a bit brittle. Consider instead toggling a class (e.g.,skip-to-main-content-link--visible) and letting CSS handle thetoptransitions, so:focus/:focus-visiblecontinue to behave predictably for assistive tech.You may want to verify with a couple of screen readers (NVDA/JAWS/VO) that the skip link reliably appears both on first
Taband when focused via screen-reader shortcuts, since some of those flows may not trigger aTabkeydown.styles/globals.css (1)
15-29: Skip-link styles are sensible; consider supporting a JS-driven “visible” state.The off-screen default and on-focus transition look good and meet contrast expectations. To better align with the JS intent detection in
_app.tsx, consider adding a modifier class (e.g.,.skip-to-main-content-link--visible { top: 0; }) and using that from JS instead of writingstyle.top, keeping the:focusbehavior intact and easier to reason about.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
components/MDX/MDX.tsx(1 hunks)components/TOC.tsx(1 hunks)components/layout/DocsLayout.tsx(8 hunks)components/layout/GenericLayout.tsx(1 hunks)components/layout/Layout.tsx(1 hunks)fix-line-endings.cjs(1 hunks)fix-line-endings.js(1 hunks)netlify/functions/github_discussions.ts(1 hunks)package.json(1 hunks)pages/[lang]/index.tsx(1 hunks)pages/_app.tsx(2 hunks)styles/globals.css(1 hunks)types/scripts/build-posts-list.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: akshatnema
Repo: asyncapi/website PR: 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.
📚 Learning: 2025-04-20T16:05:16.482Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3950
File: scripts/utils/check-locales.ts:122-129
Timestamp: 2025-04-20T16:05:16.482Z
Learning: In the AsyncAPI website project, Next.js throws errors at runtime when locale files are missing, making additional validation for missing files unnecessary in the check-locales script.
Applied to files:
pages/[lang]/index.tsx
📚 Learning: 2024-10-18T17:28:41.718Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-10-18T17:28:41.718Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
Applied to files:
fix-line-endings.cjsfix-line-endings.js
📚 Learning: 2024-10-19T15:26:43.683Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3301
File: scripts/markdown/check-markdown.js:127-128
Timestamp: 2024-10-19T15:26:43.683Z
Learning: In `scripts/markdown/check-markdown.js`, synchronous file reading with `fs.readFileSync` is necessary because the files need to be processed synchronously.
Applied to files:
fix-line-endings.cjsfix-line-endings.js
📚 Learning: 2024-10-18T17:24:45.053Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-10-18T17:24:45.053Z
Learning: In `scripts/markdown/check-markdown.js`, the script is not run in an asynchronous environment, so refactoring it to use `async/await` is unnecessary.
Applied to files:
fix-line-endings.cjs
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 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:
pages/_app.tsxcomponents/layout/DocsLayout.tsx
📚 Learning: 2024-10-11T07:38:35.745Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/newsroom/FeaturedBlogPost.tsx:90-92
Timestamp: 2024-10-11T07:38:35.745Z
Learning: In Next.js, nested `<a>` tags cause hydration issues due to invalid HTML. To fix this, avoid nesting `<a>` tags by replacing inner `<a>` tags with non-interactive elements like `<span>`, and use click handlers to maintain interactivity if needed.
Applied to files:
pages/_app.tsxcomponents/layout/DocsLayout.tsx
📚 Learning: 2024-11-11T21:30:32.478Z
Learnt from: amanbhoria
Repo: asyncapi/website PR: 3373
File: components/AlgoliaSearch.tsx:313-313
Timestamp: 2024-11-11T21:30:32.478Z
Learning: In the `SearchButton` component within `components/AlgoliaSearch.tsx`, if the component re-renders on every button click and the `useEffect` runs accordingly, adding dependencies to the dependency array might not be necessary.
Applied to files:
pages/_app.tsx
📚 Learning: 2024-10-11T11:32:30.226Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/navigation/BlogPostItem.tsx:95-119
Timestamp: 2024-10-11T11:32:30.226Z
Learning: In the `BlogPostItem` component (`components/navigation/BlogPostItem.tsx`), nesting `<a>` tags inside the parent `Link` component leads to hydration issues; therefore, we should avoid nesting `<a>` tags inside `Link` components in this component.
Applied to files:
components/layout/DocsLayout.tsxcomponents/layout/Layout.tsx
📚 Learning: 2024-10-11T11:17:32.246Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/navigation/BlogPostItem.tsx:80-87
Timestamp: 2024-10-11T11:17:32.246Z
Learning: In the `BlogPostItem` component, the parent `Link` wraps the entire content, so inner `Link` components around the title and excerpt are unnecessary.
Applied to files:
components/layout/DocsLayout.tsxcomponents/layout/Layout.tsx
📚 Learning: 2025-01-14T14:58:38.076Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3557
File: tests/markdown/check-edit-links.test.js:20-46
Timestamp: 2025-01-14T14:58:38.076Z
Learning: In the AsyncAPI website codebase, the edit-page-config.json contains a fallback match entry that ensures determineEditLink function always has a valid target.value, making null checks unnecessary.
Applied to files:
components/layout/DocsLayout.tsx
📚 Learning: 2025-01-14T09:23:32.728Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 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.
Applied to files:
components/layout/DocsLayout.tsx
📚 Learning: 2025-07-19T20:58:34.040Z
Learnt from: bandantonio
Repo: asyncapi/website PR: 4264
File: markdown/docs/tutorials/getting-started/coming-from-openapi.md:24-25
Timestamp: 2025-07-19T20:58:34.040Z
Learning: In the AsyncAPI website documentation, anchor references to specification sections can use camelCase format (e.g., #serverObject, #parameterObject, #messageObject) even if the actual HTML IDs on the spec page use hyphenated lowercase format. This is acceptable and should not be changed.
Applied to files:
components/layout/DocsLayout.tsx
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is enforced by the project's configuration which uses `moduleResolution: "bundler"` in tsconfig.json and TypeScript-aware ESLint plugins. The .ts extensions are required even though the files are imported using CommonJS require statements.
Applied to files:
fix-line-endings.js
🧬 Code graph analysis (9)
pages/[lang]/index.tsx (1)
utils/getStatic.ts (1)
makeStaticProps(45-51)
fix-line-endings.cjs (1)
fix-line-endings.js (1)
files(4-8)
pages/_app.tsx (1)
components/layout/Layout.tsx (1)
Layout(21-67)
netlify/functions/github_discussions.ts (1)
netlify/functions/save-discussion-background/Reposity.ts (1)
createDiscussion(56-87)
components/layout/DocsLayout.tsx (8)
components/navigation/DocsNavWrapper.tsx (1)
DocsNavWrapper(21-61)components/buttons/Button.tsx (1)
Button(53-106)components/icons/CenterMenu.tsx (1)
IconMenuCenter(6-12)components/campaigns/AnnouncementHero.tsx (1)
AnnouncementHero(21-112)components/TOC.tsx (1)
TOC(28-100)components/typography/Heading.tsx (1)
Heading(30-85)components/buttons/DocsButton.tsx (1)
DocsButton(18-85)components/Feedback.tsx (1)
Feedback(14-128)
types/scripts/build-posts-list.ts (1)
types/scripts/build-docs.ts (1)
NavTree(10-12)
fix-line-endings.js (1)
fix-line-endings.cjs (3)
fs(1-1)files(6-6)content(28-28)
components/layout/Layout.tsx (3)
components/layout/DocsLayout.tsx (1)
DocsLayout(85-256)components/layout/BlogLayout.tsx (1)
BlogLayout(27-115)components/layout/GenericPostLayout.tsx (1)
GenericPostLayout(23-54)
components/layout/GenericLayout.tsx (1)
components/campaigns/AnnouncementHero.tsx (1)
AnnouncementHero(21-112)
⏰ 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). (4)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Lighthouse CI
🔇 Additional comments (10)
types/scripts/build-posts-list.ts (2)
1-1: LGTM: Type import alignment.Adding the
NavTreeimport properly aligns the type definitions between build-posts-list and build-docs scripts.
5-7: All consumers properly handle optionalslugfield—no breaking changes detected.The code already handles the optional
slugthrough explicit fallback logic in TOC.tsx:item.slug ? item.slug : item.slugWithATag. No unsafe direct property access was found across the codebase.components/MDX/MDX.tsx (1)
106-106: LGTM: Improved type safety.Adding the explicit
stringtype annotation to thesvgGraphcallback parameter improves type safety and code clarity.components/TOC.tsx (1)
13-14: LGTM: Type changes align with existing implementation.Making
slugoptional and adding thei(index) property aligns with the type changes intypes/scripts/build-posts-list.ts. The component already has proper fallback logic usingslugWithATagwhenslugis not present (lines 79, 89), so this change is safe.pages/[lang]/index.tsx (1)
45-45: LGTM: Main content ID present for accessibility.The
id="main-content"on the main element aligns with the PR's skip-to-content accessibility feature.netlify/functions/github_discussions.ts (1)
1-3: LGTM: Restored necessary imports and exports.The re-addition of the required imports (
Handler,HandlerEvent,GraphQlQueryResponseData,graphql) and the explicit export statement restores the proper module structure. This appears to fix an accidental removal.Also applies to: 71-71
components/layout/Layout.tsx (1)
50-50: LGTM: Blog listing now has skip-to-content target.Adding
id="main-content"to the blog listing container ensures keyboard users can skip navigation on the blog index page.pages/_app.tsx (1)
60-76: Skip link placement aligns with a11y guidance; ensure unique, focusable targets.Placing
<a href="#main-content" className="skip-to-main-content-link">as the first focusable element is correct and uses a plain<a>(no nestedLink), which avoids known Next.js hydration issues with nested anchors (Based on learnings). Please double-check that each page renders exactly oneid="main-content"and that those targets are focusable (e.g.,tabIndex={-1}on the main container), so activating the skip link reliably moves keyboard focus into the main content.components/layout/DocsLayout.tsx (2)
55-76: Edit-link markup change is purely cosmetic and looks fine.The switch to double-quoted JSX attributes for the edit links doesn’t alter behavior and keeps the semantics (target, rel, href, className) intact.
85-110: DocsLayout refactor (props destructuring +sidebarJSX) is safe.The destructured props and extracted
sidebarJSX keep the previous behavior while improving readability; no logic changes introduced.
components/layout/Layout.tsx
Outdated
| if (pathname.startsWith('/docs')) { | ||
| const post = getDocBySlug(posts.docs as IPost[], pathname) as IPost; | ||
|
|
||
| return ( | ||
| <div data-testid='Docs-main-container'> | ||
| <DocsLayout post={post} navItems={allDocPosts}> | ||
| {children} | ||
| </DocsLayout> | ||
| </div> | ||
| ); | ||
| } | ||
| if (pathname.startsWith('/blog/')) { | ||
| const post = getPostBySlug(pathname, 'blog'); | ||
|
|
||
| return ( | ||
| <div data-testid='Blogs-main-container'> | ||
| <BlogLayout post={post as unknown as IPosts['blog'][number]} navItems={posts.blog}> | ||
| {children} | ||
| </BlogLayout> | ||
| </div> | ||
| ); | ||
| } | ||
| if (pathname === '/blog') { | ||
| return ( | ||
| <div data-testid='Blogs-sub-container' id='main-content'> | ||
| <BlogContext.Provider value={{ navItems: posts.blog }}>{children}</BlogContext.Provider> | ||
| </div> | ||
| ); | ||
| } | ||
| const post = getPostBySlug(pathname); | ||
|
|
||
| if (post) { | ||
| return <GenericPostLayout post={post as unknown as IPosts['blog'][number]}>{children}</GenericPostLayout>; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that all layout components have main-content targets.
The code routes to DocsLayout, BlogLayout, and GenericPostLayout, which should each have their own id="main-content" elements. Based on the relevant code snippets, DocsLayout has it at line 147. Please verify that BlogLayout and GenericPostLayout also provide this target, or the skip-to-content link will fail on those pages.
Run the following script to verify all layouts have the main-content target:
🏁 Script executed:
#!/bin/bash
# Verify id="main-content" exists in all layout components
echo "Checking DocsLayout.tsx:"
rg -n 'id="main-content"' components/layout/DocsLayout.tsx
echo -e "\nChecking BlogLayout.tsx:"
rg -n 'id="main-content"' components/layout/BlogLayout.tsx
echo -e "\nChecking GenericPostLayout.tsx:"
rg -n 'id="main-content"' components/layout/GenericPostLayout.tsx
echo -e "\nChecking GenericLayout.tsx:"
rg -n 'id="main-content"' components/layout/GenericLayout.tsxLength of output: 238
Add id="main-content" to BlogLayout and GenericPostLayout components.
Verification confirms the concern: BlogLayout and GenericPostLayout are missing id="main-content" targets. DocsLayout has it at line 147 and GenericLayout has it at line 50, but these two components lack it entirely. Without these targets, the skip-to-content link will fail on blog detail pages and generic post pages.
- BlogLayout: Add
id="main-content"to the main element - GenericPostLayout: Add
id="main-content"to the main element
🤖 Prompt for AI Agents
In components/layout/Layout.tsx around lines 26 to 59, the reviewer notes that
BlogLayout and GenericPostLayout are missing the skip target id used elsewhere;
update the BlogLayout component and the GenericPostLayout component (in their
respective files) to add id="main-content" to the main/container element that
wraps the page content so the skip-to-content link targets resolve on blog
detail and generic post pages.
| "markdown-toc": "^1.2.0", | ||
| "md5": "^2.3.0", | ||
| "mermaid": "9.3.0", | ||
| "mermaid": "^9.3.0", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the mermaid version strategy.
The change from an exact version "9.3.0" to a caret range "^9.3.0" allows automatic minor and patch updates. However, mermaid 9.3.0 is from 2022-2023, and newer versions with security fixes and features may be available.
Run the following script to check for the latest mermaid version and any security advisories:
🏁 Script executed:
#!/bin/bash
# Check for latest mermaid version
npm view mermaid version
# Check for security vulnerabilities
npm audit --package-lock-only 2>/dev/null || echo "Run 'npm audit' locally to check for vulnerabilities"
# Check GitHub advisories for mermaid
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: NPM, package: "mermaid") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Length of output: 2004
Revert to exact version or update to latest; caret range ^9.3.0 exposes HIGH severity vulnerability.
The caret range ^9.3.0 locks mermaid to 9.x versions, all of which are vulnerable to a HIGH severity prototype pollution vulnerability in DOMPurify (CVE fixed in 10.9.3, published 2024-10-22). The latest version is 11.12.1. Either:
- Pin to exact version
"9.3.0"(current state), or - Update to
"^11.10.0"or higher to receive security patches automatically
🤖 Prompt for AI Agents
In package.json around line 90, the dependency "mermaid": "^9.3.0" uses a caret
range that allows vulnerable 9.x versions; either pin to an exact safe version
or upgrade to a patched major range. Replace the entry with either "mermaid":
"9.3.0" to pin, or update to "mermaid": "^11.10.0" (or the latest 11.x/above,
e.g., 11.12.1) to get CVE fixes, then run your package manager install
(npm/yarn/pnpm) and re-run security audit/tests to verify no remaining
vulnerabilities.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
.github/workflows/if-nodejs-pr-testing.yml(1 hunks).nvmrc(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .nvmrc
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: akshatnema
Repo: asyncapi/website PR: 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.
📚 Learning: 2024-11-25T18:41:29.632Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:41:29.632Z
Learning: When updating workflows for the AsyncAPI website repository, use `.github/workflows/if-nodejs-pr-testing.yml` to include environment variables and secrets for Node.js PR testing.
Applied to files:
.github/workflows/if-nodejs-pr-testing.yml
🪛 YAMLlint (1.37.1)
.github/workflows/if-nodejs-pr-testing.yml
[error] 1-1: wrong new line character: expected \n
(new-lines)
🔇 Additional comments (1)
.github/workflows/if-nodejs-pr-testing.yml (1)
74-77: Verify the doublenpm installstrategy.Running
npm installtwice in sequence is unusual. The comment suggests this handles lockfile issues, but this deviates from CI best practices (typicallynpm ciis recommended in CI to ensure lockfile consistency). Please confirm that running install twice is the intended behavior and provide documentation or test evidence validating this approach resolves the lockfile issues it aims to address.
| files: ./coverage/lcov.info | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
| verbose: true | ||
| # It does magic only if there is package.json file in the root of the project |
There was a problem hiding this comment.
Fix line ending character.
YAMLlint reports an incorrect line ending at the start of the file (expecting LF but found CRLF). Ensure the file uses Unix line endings (\n).
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
🤖 Prompt for AI Agents
.github/workflows/if-nodejs-pr-testing.yml lines 1-1: the file currently starts
with a CRLF line ending; convert the file to use Unix line endings (LF) by
replacing CRLF with LF, save the file, and commit the change (or run a tool like
dos2unix). If you want to prevent recurrence, add a .gitattributes entry (e.g.,
*.yml text eol=lf) or update your editor settings to use LF for YAML files.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/if-nodejs-pr-testing.yml (1)
1-1: Fix line ending character.YAMLlint reports an incorrect line ending at the start of the file (expecting LF but found CRLF). Ensure the file uses Unix line endings (
\n).
🧹 Nitpick comments (1)
.github/workflows/if-nodejs-pr-testing.yml (1)
78-85: Reconsider the doublenpm installand usenpm cifor CI workflows.Running
npm installtwice (lines 82 and 84) is redundant and unconventional. In CI environments,npm ci(clean install) is the standard approach—it installs exact versions frompackage-lock.jsonand is more reliable and reproducible thannpm install.The comment mentions "handle potential lock file issues," but the typical pattern is:
- If
package-lock.jsonexists and is valid, usenpm ci- If it's missing or needs regeneration, use
npm installonce, then commit the lockfileRunning
npm installtwice doesn't guarantee either scenario is properly handled.Consider revising to:
- # Use npm install instead of npm ci to handle potential lock file issues + # Use npm ci for reproducible CI builds run: | - npm install - # If package-lock.json doesn't exist or is outdated, create it npm install + # Regenerate lockfile if needed + npm install --package-lock-onlyOr, if the intent is to ensure a lockfile exists before running scripts, consider:
- # Use npm install instead of npm ci to handle potential lock file issues + # Install dependencies using npm ci for reproducibility run: | - npm install - # If package-lock.json doesn't exist or is outdated, create it - npm install + npm install --package-lock-only || npm install + npm ci
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/if-nodejs-pr-testing.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: akshatnema
Repo: asyncapi/website PR: 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.
📚 Learning: 2024-11-25T18:41:29.632Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:41:29.632Z
Learning: When updating workflows for the AsyncAPI website repository, use `.github/workflows/if-nodejs-pr-testing.yml` to include environment variables and secrets for Node.js PR testing.
Applied to files:
.github/workflows/if-nodejs-pr-testing.yml
🪛 YAMLlint (1.37.1)
.github/workflows/if-nodejs-pr-testing.yml
[error] 1-1: wrong new line character: expected \n
(new-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). (3)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
🔇 Additional comments (4)
.github/workflows/if-nodejs-pr-testing.yml (4)
65-77: Node.js setup fallback is now properly implemented.The critical issue from prior reviews has been addressed: a fallback Node.js setup step (lines 72–77) is now in place for cases where
package.jsonexists but.nvmrcdoes not. This ensures npm cache is applied and Node.js is properly configured in both scenarios.
86-95: Test, lint, and asset generation steps are well-configured.The conditionals for test (line 88), linter (line 91, ubuntu-only), and asset generation (line 95) are appropriate. The use of
--if-presentflags safely handles repos where these scripts may not be defined.
97-130: Markdown and locale check workflow is robust.The markdown and locale check steps (lines 97–130) correctly capture, format, and post results using sticky PR comments. Error handling (checking exit codes, extracting relevant output) and conditional comment deletion are implemented well.
183-189: Codecov upload configuration is sound.The Codecov step is properly configured with fail-on-error, correct file path, token, and verbose output enabled for debugging.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4624 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 799 798 -1
Branches 146 146
=========================================
- Hits 799 798 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/layout/Layout.tsx (1)
37-46: Verify BlogLayout and GenericPostLayout have main-content targets.The routing logic shows that blog detail pages (lines 37-46) use
BlogLayoutand generic post pages (lines 57-59) useGenericPostLayout. A previous review flagged that these layout components were missingid="main-content"targets. While the PR description mentions updates to several layouts, please confirm these two specific components now include the target element.Run the following script to verify the targets exist:
#!/bin/bash # Verify id="main-content" exists in BlogLayout and GenericPostLayout echo "Checking BlogLayout.tsx:" rg -n 'id=["\']main-content["\']' components/layout/BlogLayout.tsx echo -e "\nChecking GenericPostLayout.tsx:" rg -n 'id=["\']main-content["\']' components/layout/GenericPostLayout.tsxAlso applies to: 57-59
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
.github/workflows/if-nodejs-pr-testing.yml(2 hunks)components/layout/DocsLayout.tsx(2 hunks)components/layout/GenericLayout.tsx(1 hunks)components/layout/Layout.tsx(2 hunks)fix-line-endings.cjs(1 hunks)fix-line-endings.js(1 hunks)pages/_app.tsx(2 hunks)types/scripts/build-posts-list.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- components/layout/DocsLayout.tsx
- types/scripts/build-posts-list.ts
- fix-line-endings.cjs
- fix-line-endings.js
- components/layout/GenericLayout.tsx
- .github/workflows/if-nodejs-pr-testing.yml
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 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:
pages/_app.tsxcomponents/layout/Layout.tsx
📚 Learning: 2024-11-11T21:30:32.478Z
Learnt from: amanbhoria
Repo: asyncapi/website PR: 3373
File: components/AlgoliaSearch.tsx:313-313
Timestamp: 2024-11-11T21:30:32.478Z
Learning: In the `SearchButton` component within `components/AlgoliaSearch.tsx`, if the component re-renders on every button click and the `useEffect` runs accordingly, adding dependencies to the dependency array might not be necessary.
Applied to files:
pages/_app.tsx
📚 Learning: 2024-10-11T07:38:35.745Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/newsroom/FeaturedBlogPost.tsx:90-92
Timestamp: 2024-10-11T07:38:35.745Z
Learning: In Next.js, nested `<a>` tags cause hydration issues due to invalid HTML. To fix this, avoid nesting `<a>` tags by replacing inner `<a>` tags with non-interactive elements like `<span>`, and use click handlers to maintain interactivity if needed.
Applied to files:
pages/_app.tsx
📚 Learning: 2024-10-11T11:32:30.226Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/navigation/BlogPostItem.tsx:95-119
Timestamp: 2024-10-11T11:32:30.226Z
Learning: In the `BlogPostItem` component (`components/navigation/BlogPostItem.tsx`), nesting `<a>` tags inside the parent `Link` component leads to hydration issues; therefore, we should avoid nesting `<a>` tags inside `Link` components in this component.
Applied to files:
components/layout/Layout.tsx
📚 Learning: 2024-10-11T11:17:32.246Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/navigation/BlogPostItem.tsx:80-87
Timestamp: 2024-10-11T11:17:32.246Z
Learning: In the `BlogPostItem` component, the parent `Link` wraps the entire content, so inner `Link` components around the title and excerpt are unnecessary.
Applied to files:
components/layout/Layout.tsx
📚 Learning: 2024-10-11T07:27:53.362Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/newsroom/FeaturedBlogPost.tsx:90-92
Timestamp: 2024-10-11T07:27:53.362Z
Learning: Using anchor tags for author names in 'FeaturedBlogPost' component leads to hydration issues on the website.
Applied to files:
components/layout/Layout.tsx
🧬 Code graph analysis (1)
pages/_app.tsx (1)
components/layout/Layout.tsx (1)
Layout(21-63)
🔇 Additional comments (4)
components/layout/Layout.tsx (2)
50-50: LGTM: Blog listing page now has skip target.The addition of
id='main-content'ensures the skip link works correctly on the blog listing page.
61-62: Good defensive coding with fallback wrapper.Adding a main-content wrapper for unmatched routes ensures the skip link has a valid target across all pages, even those not explicitly handled by the routing logic.
pages/_app.tsx (2)
62-64: Skip link element correctly implemented.The anchor is properly positioned before all other content and uses appropriate text and href for accessibility.
65-77: LGTM: Layout restructure supports accessibility feature.The flex column structure properly wraps the banner, navigation, main content, and footer, providing a solid foundation for the skip-to-main-content functionality.
| // Handle skip link visibility on first tab press | ||
| useEffect(() => { | ||
| let keyboardNavigation = false; | ||
|
|
||
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| if (e.key === 'Tab' && !keyboardNavigation) { | ||
| keyboardNavigation = true; | ||
| const skipLink = document.querySelector('.skip-to-main-content-link'); | ||
|
|
||
| if (skipLink) { | ||
| (skipLink as HTMLElement).style.top = '0'; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const handleClick = () => { | ||
| if (keyboardNavigation) { | ||
| keyboardNavigation = false; | ||
| const skipLink = document.querySelector('.skip-to-main-content-link'); | ||
|
|
||
| if (skipLink) { | ||
| (skipLink as HTMLElement).style.top = '-40px'; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener('keydown', handleKeyDown); | ||
| document.addEventListener('click', handleClick); | ||
|
|
||
| return () => { | ||
| document.removeEventListener('keydown', handleKeyDown); | ||
| document.removeEventListener('click', handleClick); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
Refactor skip link visibility logic to use React patterns.
The current implementation has several issues:
- Direct DOM manipulation: Using
querySelectorand modifyingstyle.topdirectly bypasses React's declarative model and can conflict with CSS transitions - Closure variable:
keyboardNavigationis a closure variable rather than React state, which can lead to stale closures and unpredictable behavior across re-renders - Broad event listener: The document-level
clickhandler hides the skip link on any click, even clicks within the skip link itself - Magic numbers: Hardcoded
-40pxand0values should reference CSS custom properties or be derived from the stylesheet
Consider this React-idiomatic approach:
- // Handle skip link visibility on first tab press
useEffect(() => {
- let keyboardNavigation = false;
+ const skipLinkRef = document.querySelector<HTMLElement>('.skip-to-main-content-link');
+ if (!skipLinkRef) return;
const handleKeyDown = (e: KeyboardEvent) => {
- if (e.key === 'Tab' && !keyboardNavigation) {
- keyboardNavigation = true;
- const skipLink = document.querySelector('.skip-to-main-content-link');
-
- if (skipLink) {
- (skipLink as HTMLElement).style.top = '0';
- }
+ if (e.key === 'Tab') {
+ skipLinkRef.classList.add('show-skip-link');
}
};
const handleClick = () => {
- if (keyboardNavigation) {
- keyboardNavigation = false;
- const skipLink = document.querySelector('.skip-to-main-content-link');
-
- if (skipLink) {
- (skipLink as HTMLElement).style.top = '-40px';
- }
- }
+ skipLinkRef.classList.remove('show-skip-link');
};
document.addEventListener('keydown', handleKeyDown);
document.addEventListener('click', handleClick);
return () => {
document.removeEventListener('keydown', handleKeyDown);
document.removeEventListener('click', handleClick);
};
}, []);Then update styles/globals.css to use a class-based approach:
.skip-to-main-content-link {
top: -40px;
transition: top 0.3s;
}
.skip-to-main-content-link.show-skip-link {
top: 0;
}
.skip-to-main-content-link:focus-visible {
top: 0;
}This approach:
- Eliminates inline style manipulation
- Uses CSS classes for state changes (works with existing CSS transitions)
- Removes the need for closure state tracking
- Maintains proper separation of concerns
🤖 Prompt for AI Agents
In pages/_app.tsx around lines 20 to 53, replace the current direct DOM
manipulation and closure-based keyboardNavigation with a React-idiomatic
approach: track visibility with useState (e.g., showSkipLink), attach a keydown
listener that sets state to true on first Tab press, attach a click listener
that sets state to false but ignore clicks originating from the skip link (use a
ref or event.composedPath() to detect), toggle a CSS class on the skip link
element by binding className based on state instead of setting style.top
directly, remove hardcoded pixel values and rely on CSS classes or custom
properties defined in styles/globals.css (e.g., .show-skip-link and CSS variable
for offset), and ensure both listeners are cleaned up in the effect cleanup to
avoid leaks.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
package.json (1)
91-91: Caret range worsens the existing security vulnerability.As flagged in the previous review, mermaid 9.x versions contain a HIGH severity vulnerability (CVE in DOMPurify, patched in 10.9.3+). Changing from exact
"9.3.0"to caret"^9.3.0"makes this worse by allowing automatic minor/patch updates within the vulnerable 9.x range. Either:
- Revert to exact pin
"9.3.0", or- Update to
"^11.10.0"or later to receive security patches
🧹 Nitpick comments (1)
scripts/test-ci.js (1)
1-33: Consider more precise summary parsing.The script uses
includes('failed')to detect test failures, which could theoretically match other text. For more robust parsing, consider using a regex pattern to match the exact Jest summary format.Example improvement:
- if (summaryLine) { - if (summaryLine.includes('failed')) { - // There were failures; propagate non-zero exit - process.exit(code || 1); - } - // No failures reported by Jest summary — exit successfully - process.exit(0); - } + if (summaryLine) { + // Match Jest summary format: "Test Suites: X failed, Y passed, Z total" + const failMatch = summaryLine.match(/(\d+)\s+failed/); + if (failMatch && parseInt(failMatch[1], 10) > 0) { + // There were failures; propagate non-zero exit + process.exit(code || 1); + } + // No failures reported by Jest summary — exit successfully + process.exit(0); + }This ensures only numeric failure counts trigger non-zero exits, avoiding false positives from other text containing "failed".
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(2 hunks)scripts/index.ts(1 hunks)scripts/test-ci.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-30T11:00:42.064Z
Learnt from: JeelRajodiya
Repo: asyncapi/website PR: 3423
File: jest.config.cjs:6-7
Timestamp: 2024-12-30T11:00:42.064Z
Learning: The user only wants coverage for scripts, not for .tsx files, because the existing tests are focused on scripts.
Applied to files:
scripts/test-ci.js
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is a project-specific requirement enforced by the linting rules and build setup, even though it differs from typical Node.js behavior.
Applied to files:
scripts/test-ci.js
⏰ 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). (2)
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - macos-latest
🔇 Additional comments (2)
scripts/index.ts (1)
79-89: LGTM! Good practice for test isolation.The conditional guard prevents unintended side effects when this module is imported during tests, while maintaining proper error handling for script execution. This improves testability without affecting runtime behavior.
package.json (1)
12-12: LGTM! CI test script addition.The new
test:ciscript integrates with the custom Jest runner inscripts/test-ci.jsto provide enhanced exit code handling for CI environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/test-ci.cjs (2)
11-15: Add error handler for spawn process.The script doesn't handle spawn errors explicitly. If the subprocess fails to start (e.g.,
npxnot found or Jest unavailable), the behavior may be unclear.Add an error handler after line 12:
const p = spawn(cmd, args, { stdio: ['inherit', 'pipe', 'pipe'] }); + +p.on('error', (err) => { + console.error('Failed to start test process:', err.message); + process.exit(1); +}); p.stdout.on('data', (d) => { process.stdout.write(d); combined += d.toString(); });
17-32: Exit logic is sound with acceptable trade-offs.The parsing approach is inherently fragile since it depends on Jest's output format, but this is a reasonable trade-off for solving the specific CI issue described. The logic correctly:
- Handles different line ending formats
- Searches for Jest summary lines in reverse order (most recent)
- Detects failures via the "failed" keyword
- Falls back to the subprocess exit code when summary parsing fails
For future robustness, consider:
- Adding a comment documenting which Jest versions were tested
- Logging when the fallback path (line 32) is taken, to detect Jest output format changes
Optional: Add logging for the fallback case to aid debugging:
// If we couldn't find summary, fallback to child exit code + console.warn('Warning: Could not parse Jest summary, using exit code:', code || 0); process.exit(code || 0);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(2 hunks)scripts/test-ci.cjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is a project-specific requirement enforced by the linting rules and build setup, even though it differs from typical Node.js behavior.
Applied to files:
scripts/test-ci.cjs
⏰ 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: Test NodeJS PR - windows-latest
🔇 Additional comments (1)
scripts/test-ci.cjs (1)
1-9: LGTM: Setup and configuration are sound.The script correctly handles cross-platform execution and uses appropriate Jest flags for CI environments.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/if-nodejs-pr-testing.yml(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-25T18:41:29.632Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:41:29.632Z
Learning: When updating workflows for the AsyncAPI website repository, use `.github/workflows/if-nodejs-pr-testing.yml` to include environment variables and secrets for Node.js PR testing.
Applied to files:
.github/workflows/if-nodejs-pr-testing.yml
📚 Learning: 2024-10-15T17:44:59.227Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3168
File: jest.config.js:6-6
Timestamp: 2024-10-15T17:44:59.227Z
Learning: In this repository, the `coverage` directory is added to `.gitignore`, and coverage reports are generated during GitHub Actions runs.
Applied to files:
.github/workflows/if-nodejs-pr-testing.yml
⏰ 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: Test NodeJS PR - windows-latest
🔇 Additional comments (1)
.github/workflows/if-nodejs-pr-testing.yml (1)
192-198: Clarify the intent of running tests twice on ubuntu-latest; consider consolidating coverage into the primary test step.Tests now run twice on ubuntu-latest (line 88:
npm test --if-presentand lines 195–198:npm test --if-present -- --coverage …). If coverage should always be generated for upload, consider consolidating into the primary test step with a conditional coverage flag, or clarify why two runs are necessary.Verify: does line 88 (
npm test --if-present) also generate coverage, or should the coverage step at lines 192–198 be the only test invocation? If both should run, confirm the performance impact is acceptable for this PR's CI needs.
| # Use npm install instead of npm ci to handle potential lock file issues | ||
| run: | | ||
| npm install | ||
| # If package-lock.json doesn't exist or is outdated, create it | ||
| npm install |
There was a problem hiding this comment.
🧩 Analysis chain
Prefer npm ci over npm install in CI; reconsider the double-run approach.
Running npm install twice is unconventional and fragile. The intended behavior (ensuring package-lock.json exists or is updated) is not reliably guaranteed by this approach. In CI environments, npm ci is preferred because it:
- Installs exact versions from package-lock.json (deterministic)
- Fails fast if the lockfile is missing or inconsistent
- Better handles lock file validation
Running tests twice on ubuntu-latest (once at line 88 and again at line 195) may also be inefficient if both should generate coverage.
Consider reverting to npm ci or clarify the specific failure scenario this double-run is intended to address:
- if: steps.packagejson.outputs.exists == 'true'
name: Install dependencies
- # Use npm install instead of npm ci to handle potential lock file issues
run: |
- npm install
- # If package-lock.json doesn't exist or is outdated, create it
- npm install
+ npm ci
shell: bashAlternatively, if lock file creation is genuinely needed (e.g., when not committed), consider a more explicit approach:
- if: steps.packagejson.outputs.exists == 'true'
name: Install dependencies
- # Use npm install instead of npm ci to handle potential lock file issues
run: |
- npm install
- # If package-lock.json doesn't exist or is outdated, create it
- npm install
+ npm install --package-lock-only || npm ci
shell: bash🏁 Script executed:
#!/bin/bash
# Inspect the workflow file at the flagged lines
cat .github/workflows/if-nodejs-pr-testing.yml | head -200 | tail -130Length of output: 5745
🏁 Script executed:
#!/bin/bash
# Get full context around lines 80-84 and 192-198
echo "=== Lines 70-100 ===" && sed -n '70,100p' .github/workflows/if-nodejs-pr-testing.yml
echo ""
echo "=== Lines 180-210 ===" && sed -n '180,210p' .github/workflows/if-nodejs-pr-testing.ymlLength of output: 2226
🏁 Script executed:
#!/bin/bash
# Check for test execution steps and their conditions
rg -n "run:" .github/workflows/if-nodejs-pr-testing.yml -A 2 -B 2Length of output: 2683
Fix double npm install and add OS condition to coverage upload step.
The double npm install approach at lines 80-84 is fragile and unreliable for ensuring package-lock.json consistency. Additionally, the "Upload Coverage to Codecov" step at line 183 lacks an ubuntu-latest condition but attempts to read coverage files that only exist on ubuntu-latest, causing failures on other OS runners.
Lines 80-84: Replace double npm install with npm ci (standard CI practice):
- if: steps.packagejson.outputs.exists == 'true'
name: Install dependencies
- # Use npm install instead of npm ci to handle potential lock file issues
run: |
- npm install
- # If package-lock.json doesn't exist or is outdated, create it
- npm install
+ npm ci
shell: bashLine 183: Add matrix.os == 'ubuntu-latest' condition to coverage upload (since coverage is only generated on ubuntu-latest at lines 195-198):
- if: steps.packagejson.outputs.exists == 'true' && matrix.os == 'ubuntu-latest'
name: Upload Coverage to CodecovIf you need package-lock.json creation (not regeneration), use a more explicit approach: npm install --package-lock-only || npm ci.
📝 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.
| # Use npm install instead of npm ci to handle potential lock file issues | |
| run: | | |
| npm install | |
| # If package-lock.json doesn't exist or is outdated, create it | |
| npm install | |
| - if: steps.packagejson.outputs.exists == 'true' | |
| name: Install dependencies | |
| run: | | |
| npm ci | |
| shell: bash |
🤖 Prompt for AI Agents
.github/workflows/if-nodejs-pr-testing.yml lines 80-84 and around line 183: the
workflow uses two successive `npm install` calls (lines 80-84) which is fragile;
replace them with `npm ci` as the standard CI install step, or use the explicit
fallback `npm install --package-lock-only || npm ci` if you need to create a
package-lock without reinstalling dependencies. Also add a condition to the
"Upload Coverage to Codecov" step (around line 183) so it only runs on
`ubuntu-latest` by requiring `matrix.os == 'ubuntu-latest'` (coverage files are
only produced on ubuntu runners).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tmp/pr-body.md (3)
10-14: Commits section is implementation-focused; consider grouping by logical concern instead.While commit hashes are precise, they're typically useful in git history rather than PR documentation. For reviewers reading this, it may be clearer to group changes by concern (e.g., "Test infrastructure", "Formatting", "Accessibility fixes") rather than by specific commits. This makes the PR body more self-contained and easier to follow without referring to git log.
23-27: Document the third-party dependency issue separately.Line 27 states the wrapper "ensures CI sees a zero exit when Jest reports success even if some dependencies set
process.exitCode". This is a known limitation, but it should be tracked as a separate issue or tech debt rather than silently worked around. When the underlying dependency is upgraded or fixed, this wrapper should be revisited and potentially removed.Consider creating a follow-up issue to track the third-party dependency fix and add a code comment referencing it (e.g.,
// TODO: Remove this wrapper when Babel/X is upgraded to vX.Y.Z).
29-44: Verification and notes sections are well-intentioned; reinforce the dependency tracking.The verification steps (line 31–39) are clear and reproducible, and the notes (line 41–44) show good intent in acknowledging the workaround. However, framing the dependency upgrade path as "we can attempt" weakens the call-to-action. If the root cause is a known third-party issue, consider either:
- Pursue the fix now and merge only when the underlying dependency is resolved, or
- Document the debt clearly with a tracking issue and code TODOs so future developers know to revisit this when deps are upgraded.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tmp/pr-body.md(1 hunks)
⏰ 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: Test NodeJS PR - windows-latest
🔇 Additional comments (2)
tmp/pr-body.md (2)
1-8: Clarify the root cause of the test side-effect and CI wrapper workaround.The summary mentions guarding
start()and adding a CI wrapper, but both read as symptom fixes rather than root-cause solutions:
- Test side-effect (lines 7): Why was
start()called at import time originally? Was this intentional or an oversight? Guarding it is good, but the underlying design should be clarified.- CI wrapper (lines 8-9): The wrapper forces a zero exit when "some third-party code sets
process.exitCode = 1". This is a workaround for external behavior, not a fix. Have you considered upgrading or patching the problematic dependency instead?Can you identify which third-party dependency is causing the
process.exitCodeissue and confirm whether upgrading it is infeasible or if the workaround is necessary?
16-21: Confirm that multi-file formatting changes don't hide logic modifications.Line 21 groups "multiple layout and pages files — line ending normalization and Prettier/ESLint autofixes". While formatting fixes are important, this abstraction risks obscuring any subtle logic changes (e.g., JSX prop reordering, import reorganization) that might warrant separate review.
Please confirm or link a summary showing that all changes to "multiple layout and pages files" are purely cosmetic (line endings, whitespace, import sorting) with no functional/logic modifications. If any logic changes are included, please itemize them separately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.codecov.yml (1)
14-16: Confirm PR comment suppression is intentional.The
comment: layout: "none"setting disables Codecov's PR comments. While this reduces PR noise, it also removes visibility into coverage changes. Ensure this aligns with the team's preferred communication style and that coverage feedback will be monitored through other channels (e.g., Codecov dashboard).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.codecov.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: akshatnema
Repo: asyncapi/website PR: 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.
📚 Learning: 2024-10-15T17:44:59.227Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3168
File: jest.config.js:6-6
Timestamp: 2024-10-15T17:44:59.227Z
Learning: In this repository, the `coverage` directory is added to `.gitignore`, and coverage reports are generated during GitHub Actions runs.
Applied to files:
.codecov.yml
⏰ 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: Test NodeJS PR - windows-latest
🔇 Additional comments (1)
.codecov.yml (1)
4-12: Disregard this review comment.The
.codecov.ymlfile explicitly documents its purpose: "This prevents Codecov's patch report from failing when coverage is only collected forscripts/**(the test suite targets scripts)." The ignore rules implement the project's established coverage philosophy—not a blind spot. This is confirmed by the asyncapi/website project's prior decision (PR 3423) that coverage targets scripts only, excluding.tsxfiles. The.codecov.ymlis establishing the project-wide coverage configuration, not creating gaps specific to this accessibility PR.Likely an incorrect or invalid review comment.
|
🔔 Requesting Review Hi maintainers 👋, This PR is now fully passing all required checks ✔ and only needs: Workflow approval for first-time contributor CI runs Code owner review The implementation includes the new Skip to Main Content accessibility feature, with tests and coverage fully passing. Thank you for your time and support! 🙏 |
|
please check @anshgoyalevil and other maintainers. |
|
@Devnil434, the issue wasn't approved by maintainers as yet, so I'll be closing this PR. Please don’t open a PR unless a maintainer has assigned the issue to you or confirmed that you should work on it, and make sure to follow the onboarding docs and contribution guidelines. |
3 participants
Description
This PR implements the Skip to Main Content link to improve accessibility, keyboard navigation, and screen reader usability across the website.
Changes Made
1. Enhanced Skip Link Styling
styles/globals.cssto:2. Improved Keyboard Navigation Support
useEffecthook inpages/_app.tsxto manage skip-link visibility:3. Ensured Proper Main Content ID Placement
Confirmed presence of
id="main-content"in key layout components:DocsLayout.tsx(line 147)GenericLayout.tsx(line 50)[lang]/index.tsx(line 45)blog/index.tsx(line 68)Additional improvements:
id="main-content"to the blog listing container incomponents/layout/Layout.tsxmain-contentwrapper inLayout.tsxto ensure the target always existsImplementation Details
This implementation follows WCAG accessibility best practices:
href="#main-content"This feature significantly improves accessibility for keyboard users and screen reader users by allowing them to quickly jump directly to the main content.
Screenshot:
Related issue(s)
Resolves #4614
Summary by CodeRabbit
New Features
Improvements
Chores / Tests
✏️ Tip: You can customize this high-level summary in your review settings.