feat: refine video facade and enhance YouTube embed experience #4939
Conversation
Anexus5919
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg,
princerajpoot20 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. |
![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.
📝 WalkthroughWalkthroughAdds a new responsive Changes
Sequence DiagramsequenceDiagram
actor User
participant RoadmapPage
participant Modal
participant YouTubeEmbed
participant YouTubeIFrameAPI as "YouTube IFrame API"
User->>RoadmapPage: Click video thumbnail (or press Enter/Space)
RoadmapPage->>RoadmapPage: setCurrentVideoId(id), setVideoModalOpen(true)
RoadmapPage->>Modal: render with currentVideoId
Modal->>YouTubeEmbed: mount with videoId & appendSrc
YouTubeEmbed->>YouTubeIFrameAPI: check/load API script
alt API loads (or already loaded)
YouTubeIFrameAPI-->>YouTubeEmbed: onAPIReady
end
YouTubeEmbed->>YouTubeIFrameAPI: create YT.Player with playerVars
YouTubeIFrameAPI-->>YouTubeEmbed: player ready
YouTubeEmbed->>YouTubeEmbed: seek to start (if provided) and play
User->>Modal: Close modal
Modal->>RoadmapPage: onModalClose
RoadmapPage->>YouTubeEmbed: unmount
YouTubeEmbed->>YouTubeIFrameAPI: destroy player
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4939--asyncapi-website.netlify.app/ |
|
@princerajpoot20 Please review this PR to close #4891 when you have time. Thank you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @components/YouTubeEmbed.tsx:
- Around line 16-19: The YouTubeEmbed component currently destructures a unused
title prop and maintains redundant state currentVideoId that just mirrors id;
remove the useState for currentVideoId and use id directly (or if you truly need
a stable mutable holder, replace it with a useRef) and update all references of
currentVideoId to id (e.g., where videoId is assigned and in effect dependency
arrays), and either use the title prop for accessibility (set it as the
iframe/container title or aria-label) or remove title from the prop list.
- Line 82: YouTubeEmbed currently hardcodes id="youtube-player" causing
collisions when multiple instances mount; change the component to generate a
unique id per instance (e.g., via React's useId() or a useRef(uuid)) and use
that unique id for the div and wherever the YouTube API/player initialization
references the DOM node; update references to containerRef and any player-init
code to use the generated uniqueId instead of the fixed string so each
YouTubeEmbed instance targets its own element.
- Around line 64-70: The component currently overwrites the global
window.onYouTubeIframeAPIReady which causes race conditions; instead register
createPlayer with a shared callback queue: if window.YT && window.YT.Player call
createPlayer immediately; otherwise ensure a global array (e.g.,
window.__ytInitCallbacks) exists, push createPlayer into that array, and have
the single bootstrapper (the script that sets onYouTubeIframeAPIReady) iterate
and call all queued callbacks when the API is ready; do not overwrite
window.onYouTubeIframeAPIReady directly—use the shared array or attach an event
listener so multiple YouTubeEmbed instances can initialize safely.
In @pages/roadmap.tsx:
- Around line 2-3: Imports in pages/roadmap.tsx are not grouped/ordered per the
linter; ensure external modules (React) are separated from local modules
(YouTubeEmbed) with the correct grouping and ordering and a blank line between
groups, or simply run npx eslint --fix pages/roadmap.tsx to auto-sort; verify
the imports for React and YouTubeEmbed follow project import ordering
conventions after the fix.
- Around line 686-692: The JSX attribute line is over 120 chars and uses double
quotes; change all JSX string attributes here (e.g., the className value and any
other quoted JSX props) to single quotes per project convention and break the
long attribute into multiple lines or extract it to a constant (e.g.,
videoCardClass) and reference that in the element to keep the JSX line under 120
chars; ensure the onClick handler calling setCurrentVideoId and
setVideoModalOpen remains unchanged, then run npx prettier --write to normalize
quote style across the file.
🧹 Nitpick comments (1)
components/YouTubeEmbed.tsx (1)
77-78: Add an explanatory note to the eslint-disable comment.Based on learnings from this repository, when intentionally omitting dependencies, include a reason to help reviewers understand the design choice.
📝 Suggested change
- // eslint-disable-next-line react-hooks/exhaustive-deps + // eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally omitting appendSrc to avoid recreating the player when only start time changes }, [currentVideoId]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/YouTubeEmbed.tsxpages/roadmap.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.
Applied to files:
components/YouTubeEmbed.tsxpages/roadmap.tsx
🧬 Code graph analysis (1)
pages/roadmap.tsx (3)
components/Modal.tsx (1)
Modal(15-77)components/YouTubeEmbed.tsx (1)
YouTubeEmbed(16-85)components/Warning.tsx (1)
Warning(18-36)
🪛 GitHub Actions: PR testing - if Node project
components/YouTubeEmbed.tsx
[warning] 16-16: Missing JSDoc comment.
[error] 16-16: 'title' is assigned a value but never used. (no-unused-vars)
pages/roadmap.tsx
[error] 2-3: Run autofix to sort these imports! simple-import-sort/imports. Also: import/order.
[error] 687-687: This line has a length of 126. Maximum allowed is 120. max-len.
[error] 687-687: Prettier: Replace double quotes with single quotes in JSX attributes. jsx-quotes.
⏰ 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 (3)
pages/roadmap.tsx (3)
491-492: LGTM!State management for modal visibility and current video ID is straightforward and appropriate for this use case.
707-720: Good handling of nested interactive elements.Using
e.stopPropagation()on the channel links prevents accidentally triggering the parent click handler. Thepointer-events-autoclass correctly re-enables clicks on the anchors within thepointer-events-nonecontainer.
730-734: VerifyappendSrcformat expectation.The
appendSrcprop includes the leading?('?start=86'), butYouTubeEmbedonly checks forstart=substring. This works, but consider documenting the expected format or stripping/normalizing the leading?in the component.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @components/YouTubeEmbed.tsx:
- Around line 74-78: The cleanup closure currently references the local player
variable which may be undefined due to async creation; update the cleanup to
read from playerRef.current instead (the same ref used by
createPlayer/onYouTubeIframeAPIReady) so it always destroys the actual mounted
player instance, e.g., check playerRef.current and call its destroy method if
present; ensure any place that assigns the player uses playerRef.current =
createdPlayer so the ref is authoritative.
- Around line 15-18: Add a JSDoc block for the YouTubeEmbed function and remove
the unused state indirection: delete the useState-created currentVideoId and use
the incoming id prop directly everywhere (replace currentVideoId with id in the
videoId assignment and in any dependency arrays such as the effect that watches
videoId), and update the function signature/comments accordingly so the function
has proper documentation and no unnecessary state.
- Around line 66-72: The component currently overwrites
window.onYouTubeIframeAPIReady which causes races when multiple YouTubeEmbed
instances mount; instead implement a callback queue: create or reuse a global
array (e.g. window.__YT_ON_READY_CALLBACKS) and a single dispatcher for
window.onYouTubeIframeAPIReady that drains and calls all queued callbacks, then
in YouTubeEmbed push your createPlayer callback into that array (and if
window.YT && window.YT.Player is already present, call createPlayer
immediately); ensure you don't overwrite an existing dispatcher when
initializing the queue so concurrent mounts safely register their callbacks.
- Around line 21-30: When creating the YouTube player instance (the assignment
to player using window.YT.Player with containerRef.current and currentVideoId),
add the host option set to 'https://www.youtube-nocookie.com' to the constructor
options object alongside height, width, videoId and playerVars so the player
uses the privacy-enhanced no-cookie domain; keep loading the iframe API from
youtube.com as-is but include host: 'https://www.youtube-nocookie.com' in the
options passed to window.YT.Player.
In @pages/roadmap.tsx:
- Line 757: Add a trailing newline at the end of pages/roadmap.tsx (ensure file
ends with a single newline character) so the file ends with a newline to satisfy
Prettier/ESLint and the pipeline; save the file so VCS records the EOF newline.
- Line 2: The import line "import React, { useState } from 'react';" in
pages/roadmap.tsx is out of sort order; reorder the imports in that file to
match the project's import-sorting rules (grouping and alphabetical order) or
run the repository autofix (eslint/formatter autofix) to apply the correct order
so the import sorting linter passes.
- Around line 686-735: The video thumbnail div uses only onClick (setting
setCurrentVideoId('u83V2gIUGHU') and setVideoModalOpen(true)), making it
inaccessible to keyboard users; update the container (the element with the
onClick that calls setCurrentVideoId and setVideoModalOpen) to include
role="button" and tabIndex={0}, and add an onKeyDown handler that listens for
Enter (key === 'Enter') and Space (key === ' ' or key === 'Spacebar') to invoke
the same actions; ensure the existing anchor onClick stopPropagation logic still
works (do not change those handlers) and keep pointer-events behavior so
mouse/touch interaction is unchanged.
- Around line 709-720: Prettier is flagging formatting on the JSX anchor and img
elements (the <a> elements with onClick={(e) => e.stopPropagation()} and the
<img> element with src/alt/className); run Prettier autofix (or reformat those
JSX attributes) so long attribute lists are wrapped per Prettier rules—place
each prop on its own line where required, ensure the img is self-closing and
there are no trailing spaces, and keep the onClick handlers and className values
intact.
🧹 Nitpick comments (2)
components/YouTubeEmbed.tsx (1)
79-80: Add explanatory note to the eslint-disable comment.Based on learnings from this repository, when intentionally omitting dependencies in React hooks, the eslint-disable comment should include an explanatory note. Additionally,
appendSrcis used inside the effect but not listed in the dependency array.Proposed fix
- // eslint-disable-next-line react-hooks/exhaustive-deps - }, [currentVideoId]); + // eslint-disable-next-line react-hooks/exhaustive-deps -- appendSrc is intentionally omitted; seek is only needed on initial player creation + }, [id]);pages/roadmap.tsx (1)
737-741: Consider extracting the video ID and start time as constants.The video ID
u83V2gIUGHUis duplicated (lines 690 and 739 via state), and the start time86is a magic number. Extracting these as named constants improves maintainability.Proposed refactor
// Near the top of the component or as module constants const ROADMAP_VIDEO_ID = 'u83V2gIUGHU'; const ROADMAP_VIDEO_START_SECONDS = 86;Then use
ROADMAP_VIDEO_IDin the onClick handler andappendSrc={start=${ROADMAP_VIDEO_START_SECONDS}}in the YouTubeEmbed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/YouTubeEmbed.tsxpages/roadmap.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: asyncapi-bot
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.
Applied to files:
pages/roadmap.tsxcomponents/YouTubeEmbed.tsx
🧬 Code graph analysis (1)
pages/roadmap.tsx (4)
components/Modal.tsx (1)
Modal(15-77)components/YouTubeEmbed.tsx (1)
YouTubeEmbed(15-87)components/Warning.tsx (1)
Warning(18-36)components/layout/GenericLayout.tsx (1)
GenericLayout(25-51)
🪛 GitHub Actions: PR testing - if Node project
pages/roadmap.tsx
[error] 2-2: Run autofix to sort these imports! simple-import-sort/imports
[error] 709-709: Prettier formatting error: Replace long attribute sequence as suggested by prettier/prettier
[error] 717-717: Prettier formatting error: Replace long attribute sequence as suggested by prettier/prettier
[error] 725-725: Prettier formatting error: Insert newline according to prettier/prettier
[error] 726-726: Prettier formatting error: Delete '>' as suggested by prettier/prettier
[error] 727-727: Prettier formatting error: Replace ··<svg with > according to prettier/prettier
[error] 728-728: Prettier formatting error: Replace attribute sequence with properly formatted multi-line JSX as suggested by prettier/prettier
[error] 741-741: Prettier formatting error: Delete newline as suggested by prettier/prettier
[error] 757-757: Prettier formatting error: Newline required at end of file (eol-last)
[error] 757-757: Prettier formatting error: Newline required at end of file (eol-last)
components/YouTubeEmbed.tsx
[warning] 15-15: Missing JSDoc comment. require-jsdoc
🔇 Additional comments (1)
pages/roadmap.tsx (1)
491-492: LGTM on the modal state management.The state hooks for controlling modal visibility and tracking the current video ID are appropriately placed and follow React conventions.
| useEffect(() => { | ||
| if (!window.YT) { | ||
| const tag = document.createElement('script'); | ||
|
|
||
| tag.src = 'https://www.youtube.com/iframe_api'; | ||
| const firstScriptTag = document.getElementsByTagName('script')[0]; | ||
|
|
||
| firstScriptTag.parentNode?.insertBefore(tag, firstScriptTag); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
YouTube IFrame API youtube-nocookie.com privacy enhanced mode playerVars host property
💡 Result:
Short answer: Yes — when creating a YT.Player you can force the iframe to use the privacy-enhanced domain by supplying a host option (set to "https://www.youtube-nocookie.com"). Example:
var player = new YT.Player('player', {
videoId: 'M7lc1UVf-VE',
host: 'https://www.youtube-nocookie.com',
playerVars: { /* ... */ }
});
Notes:
- The IFrame API docs show the YT.Player constructor and playerVars usage. [1]
- The host option is used in practice to enforce youtube-nocookie embeds (examples/discussion). [2]
- YouTube’s official Help page documents Privacy‑Enhanced Mode (use youtube-nocookie.com), but it also warns privacy is limited to the embedded-player context (and some API scripts or network calls may still involve youtube.com). [3]
Sources:
[1] Google Developers — IFrame Player API reference.
[2] Community examples / discussion on enforcing nocookie with YT.Player (StackOverflow).
[3] YouTube Help — “Turn on privacy‑enhanced mode” (youtube-nocookie.com).
🏁 Script executed:
cat -n components/YouTubeEmbed.tsxRepository: asyncapi/website
Length of output: 2987
Add host: 'https://www.youtube-nocookie.com' to the YT.Player constructor options for privacy-enhanced embedding.
The PR objective (issue #4891) requires privacy-enhanced YouTube embeds. While the IFrame API script must load from youtube.com, the player itself can be configured to use the no-cookie domain by adding the host property at lines 38-61:
player = new window.YT.Player(containerRef.current, {
height: '100%',
width: '100%',
videoId: currentVideoId,
host: 'https://www.youtube-nocookie.com',
playerVars: {
rel: 0,
modestbranding: 1,
enablejsapi: 1,
origin: window.location.origin,
autoplay: 1
},
events: { /* ... */ }
});🤖 Prompt for AI Agents
In @components/YouTubeEmbed.tsx around lines 21 - 30, When creating the YouTube
player instance (the assignment to player using window.YT.Player with
containerRef.current and currentVideoId), add the host option set to
'https://www.youtube-nocookie.com' to the constructor options object alongside
height, width, videoId and playerVars so the player uses the privacy-enhanced
no-cookie domain; keep loading the iframe API from youtube.com as-is but include
host: 'https://www.youtube-nocookie.com' in the options passed to
window.YT.Player.
pages/roadmap.tsx
Outdated
| </GenericLayout> | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add newline at end of file.
The pipeline reports a missing newline at end of file. This is typically auto-fixed by Prettier/ESLint.
🧰 Tools
🪛 GitHub Actions: PR testing - if Node project
[error] 757-757: Prettier formatting error: Newline required at end of file (eol-last)
[error] 757-757: Prettier formatting error: Newline required at end of file (eol-last)
🤖 Prompt for AI Agents
In @pages/roadmap.tsx at line 757, Add a trailing newline at the end of
pages/roadmap.tsx (ensure file ends with a single newline character) so the file
ends with a newline to satisfy Prettier/ESLint and the pipeline; save the file
so VCS records the EOF newline.
e1786f8 to
13ca688
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @components/YouTubeEmbed.tsx:
- Line 84: The div inside YouTubeEmbed uses a hardcoded id 'youtube-player'
which can duplicate across instances; remove the id attribute (leave the ref
containerRef on the div) or replace it with a per-instance unique id prop (e.g.,
accept a playerId prop or generate one in the component) so the YouTube Player
constructor continues to use containerRef.current without relying on a global
id; update any code that referenced 'youtube-player' to use the ref or the new
unique id.
- Around line 66-72: The current logic in YouTubeEmbed assigns
window.onYouTubeIframeAPIReady directly, which overwrites the global callback
when multiple component instances mount; instead implement a queue or
event-based registration so each instance registers its createPlayer callback
(e.g., push callbacks into a shared array like youtubeInitQueue) and set a
single window.onYouTubeIframeAPIReady once that drains the queue and invokes all
queued createPlayer functions; also have instances check window.YT &&
window.YT.Player to run immediately and remove their callback from the queue on
unmount to avoid leaks.
In @pages/roadmap.tsx:
- Around line 69-77: The external anchor linking to
"https://twitter.com/ScriptedAlchemy/status/1373743197453656065" (the link text
"this tweet") opens in a new tab via target="_blank" but lacks rel="noopener
noreferrer"; update that <a> element in pages/roadmap.tsx to include
rel="noopener noreferrer" alongside target="_blank" to prevent window.opener
access and fix the security/static-analysis warning.
- Line 2: Imports in pages/roadmap.tsx are mis-sorted; run the project's lint
autofix (e.g., npm run lint:fix or npx eslint --fix pages/roadmap.tsx) to
reorder the import statements so they comply with the configured import-sorting
rule, then stage the modified file (pages/roadmap.tsx) and update the PR.
🧹 Nitpick comments (2)
components/YouTubeEmbed.tsx (2)
15-18: Redundant state initialization from prop.
currentVideoIdis initialized fromidbut never updated. If theidprop changes, the component won't reflect the new video. Either useiddirectly or sync state with the prop.♻️ Suggested fix: Use prop directly
export default function YouTubeEmbed({ id, appendSrc = '' }: YouTubeEmbedProps) { const containerRef = useRef<HTMLDivElement>(null); const playerRef = useRef<any>(null); - const [currentVideoId] = useState(id);Then replace
currentVideoIdwithidin the dependency array andvideoIdassignment.
79-80: Add explanatory note to eslint-disable comment.Per repository conventions, when intentionally omitting dependencies in React hooks, include a brief justification in the eslint-disable comment.
Based on learnings, the comment should explain why
appendSrcis intentionally omitted.♻️ Suggested fix
- // eslint-disable-next-line react-hooks/exhaustive-deps + // eslint-disable-next-line react-hooks/exhaustive-deps -- appendSrc is only needed on initial mount; re-seeking on change is not desired }, [currentVideoId]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/YouTubeEmbed.tsxpages/roadmap.tsx
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: asyncapi-bot
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.
Applied to files:
components/YouTubeEmbed.tsxpages/roadmap.tsx
📚 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/roadmap.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/roadmap.tsx
📚 Learning: 2024-11-25T18:34:51.303Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:34:51.303Z
Learning: When reviewing `scripts/markdown/check-markdown.js`, optimizations should be addressed in separate issues and not included in the current pull request.
Applied to files:
pages/roadmap.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:
pages/roadmap.tsx
🧬 Code graph analysis (1)
pages/roadmap.tsx (7)
components/layout/GenericLayout.tsx (1)
GenericLayout(25-51)components/typography/Paragraph.tsx (1)
Paragraph(28-56)components/typography/TextLink.tsx (1)
TextLink(21-42)components/InlineHelp.tsx (1)
InlineHelp(17-47)components/roadmap/RoadmapColumn.tsx (1)
RoadmapColumn(27-47)components/Modal.tsx (1)
Modal(15-77)components/YouTubeEmbed.tsx (1)
YouTubeEmbed(15-87)
🪛 Biome (2.1.2)
pages/roadmap.tsx
[error] 71-71: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
🪛 GitHub Actions: PR testing - if Node project
components/YouTubeEmbed.tsx
[warning] 15-15: Missing JSDoc comment. (require-jsdoc)
pages/roadmap.tsx
[error] 2-2: Run autofix to sort these imports! (simple-import-sort/imports)
[error] 46-46: Prettier formatting error. Replace and reflow line to satisfy prettier/prettier rule.
[error] 51-51: Prettier formatting error. Replace and reflow line to satisfy prettier/prettier rule.
[error] 56-56: Prettier formatting error. Replace and reflow line to satisfy prettier/prettier rule.
[error] 60-60: Prettier formatting error. Replace and reflow line to satisfy prettier/prettier rule.
[error] 66-66: Prettier formatting error. Replace and reflow line to satisfy prettier/prettier rule.
[error] 69-69: Prettier formatting error. Replace and reflow line to satisfy prettier/prettier rule.
[error] 70-70: Prettier formatting error. Replace and reflow line to satisfy prettier/prettier rule.
[error] 75-75: Prettier formatting error. Replace and reflow line to satisfy prettier/prettier rule.
[error] 80-80: Prettier formatting error. Replace and reflow line to satisfy prettier/prettier rule.
[error] 85-85: Prettier formatting error. Replace and reflow line to satisfy prettier/prettier rule.
[error] 92-92: Prettier formatting error. Replace and reflow line to satisfy prettier/prettier rule.
[error] 98-98: Prettier formatting error. Replace and reflow line to satisfy prettier/prettier rule.
[error] 104-104: Prettier formatting error. Replace and reflow line to satisfy prettier/prettier rule.
[error] 110-110: Prettier formatting error. Replace and reflow line to satisfy prettier/prettier rule.
⏰ 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 (3)
pages/roadmap.tsx (3)
627-628: LGTM! State management for modal.The state variables for controlling video modal visibility and current video ID are properly declared using
useState.
885-952: Well-structured thumbnail card with proper event handling.The thumbnail implementation correctly uses
stopPropagation()on nested anchor tags to prevent modal opening when clicking channel links. The play button overlay and gradient styling provide a polished UX.
954-961: Modal integration looks good.The conditional rendering of the modal with
YouTubeEmbedcorrectly passes the video ID and start time parameter. TheonModalClosehandler properly resets state.One minor note: the
appendSrcprop value"?start=86"includes the?prefix, but the component only looks forstart=pattern, so this works. Consider documenting this expected format in the component's interface.
Anexus5919
force-pushed
the
feature/refine-video-facade
branch
from
13ca688 to
c02df00
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @components/YouTubeEmbed.tsx:
- Around line 66-72: The code in YouTubeEmbed overwrites
window.onYouTubeIframeAPIReady which causes a race when multiple YouTubeEmbed
instances mount; change the initialization to register callbacks into a shared
queue instead of replacing the global handler: create or use a global array like
window.__YT_IFRAME_READY_CALLBACKS and push createPlayer for each component (and
call it immediately if window.YT.Player exists), then ensure the global
onYouTubeIframeAPIReady invokes all queued callbacks (and clears the queue);
update the YouTubeEmbed mounting logic to push/createPlayer and not assign
window.onYouTubeIframeAPIReady directly so multiple instances initialize
correctly.
- Around line 15-18: The YouTubeEmbed component captures the incoming id in a
one-time state variable currentVideoId, so when the id prop changes (e.g., modal
reused) the player never updates; fix by removing the one-time state and use the
id prop directly in the component (or, if state is required, add a useEffect in
YouTubeEmbed to sync currentVideoId whenever the id prop changes and then
trigger the player update using playerRef). Ensure you update references from
currentVideoId to id (or update state in the effect) and call the player update
logic inside the effect so the video switches when id changes.
In @pages/roadmap.tsx:
- Around line 46-160: The review flags many Prettier formatting errors in this
file (affecting JSX blocks like the objects createRoadmapLandingPage,
createPluginsForOtherFrameworks, makeGeneratorGenerateModels,
makeGeneratorSupportReact, createOutstandingDocs, and surrounding JSX
fragments); fix by running the project's formatter (e.g., npm run format) or
your configured Prettier command, commit the reformatted changes, and ensure the
file passes lint/CI; if any manual adjustments are required, reformat the JSX
fragments and trailing commas/quotes to match Prettier rules around those named
entries.
- Line 2: The import ordering in pages/roadmap.tsx is failing lint rules (the
current line "import React, { useState } from 'react';"); reorder imports to
match the project's import-sort rules or run the autofixer: run `npm run
lint:fix` (or the configured import-sort autofix) and commit the updated import
order; ensure the React import and any other imports in roadmap.tsx follow the
configured grouping and alphabetical order so the pipeline lint check passes.
- Around line 69-77: The external anchor element that opens a new tab (the <a>
with href "https://twitter.com/ScriptedAlchemy/status/1373743197453656065" and
target="_blank") must include rel="noopener noreferrer" to prevent the opened
page from accessing window.opener; update that anchor in the roadmap component
to add rel="noopener noreferrer" (keeping the existing href and target
attributes unchanged).
🧹 Nitpick comments (3)
components/YouTubeEmbed.tsx (2)
79-80: Add explanatory note to eslint-disable comment.Based on learnings, when intentionally omitting dependencies in React hooks, add an explanation to justify the design choice.
Proposed fix
- // eslint-disable-next-line react-hooks/exhaustive-deps + // eslint-disable-next-line react-hooks/exhaustive-deps -- appendSrc is intentionally omitted; it's only read on initial player ready event
15-15: Add JSDoc comment to address pipeline warning.The pipeline reports a missing JSDoc comment for this exported function.
Proposed fix
+/** + * YouTubeEmbed component that renders a YouTube video player using the IFrame API. + * @param props - Component props containing video id and optional appendSrc for start time. + */ export default function YouTubeEmbed({ id, appendSrc = '' }: YouTubeEmbedProps) {pages/roadmap.tsx (1)
954-961: Modal title is empty and video embed approach looks correct.The modal implementation properly uses the
YouTubeEmbedcomponent with the video ID and start time. However, the emptytitle=''prop may impact accessibility—screen readers rely on modal titles.Proposed fix
<Modal - title='' + title='AsyncAPI SIG Meeting Recording' onModalClose={() => setVideoModalOpen(false)} >
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/YouTubeEmbed.tsxfix-quotes.jspages/roadmap.tsx
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: asyncapi-bot
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
📚 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-quotes.js
📚 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-quotes.js
📚 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/roadmap.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/roadmap.tsx
📚 Learning: 2024-11-25T18:34:51.303Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:34:51.303Z
Learning: When reviewing `scripts/markdown/check-markdown.js`, optimizations should be addressed in separate issues and not included in the current pull request.
Applied to files:
pages/roadmap.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:
pages/roadmap.tsx
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.
Applied to files:
pages/roadmap.tsxcomponents/YouTubeEmbed.tsx
🧬 Code graph analysis (1)
pages/roadmap.tsx (4)
components/layout/GenericLayout.tsx (1)
GenericLayout(25-51)components/roadmap/RoadmapColumn.tsx (1)
RoadmapColumn(27-47)components/Modal.tsx (1)
Modal(15-77)components/YouTubeEmbed.tsx (1)
YouTubeEmbed(15-87)
🪛 Biome (2.1.2)
pages/roadmap.tsx
[error] 71-71: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
🪛 GitHub Actions: PR testing - if Node project
pages/roadmap.tsx
[error] 2-2: Error: Run autofix to sort these imports! simple-import-sort/imports
[error] 46-46: Prettier formatting issue detected. Replace the line break as suggested by prettier/prettier.
[error] 51-51: Prettier formatting issue detected. Replace the line break as suggested by prettier/prettier.
[error] 56-56: Prettier formatting issue detected. Replace the line break as suggested by prettier/prettier.
[error] 60-60: Prettier formatting issue detected. Replace the line break as suggested by prettier/prettier.
[error] 66-66: Prettier formatting issue detected. Replace the line break as suggested by prettier/prettier.
[error] 69-69: Prettier formatting issue detected. Replace the line break as suggested by prettier/prettier.
[error] 70-70: Prettier formatting issue detected. Replace the line break as suggested by prettier/prettier.
[error] 71-71: Prettier formatting issue detected. Replace the line break as suggested by prettier/prettier.
[error] 75-75: Prettier formatting issue detected. Replace the line break as suggested by prettier/prettier.
[error] 80-80: Prettier formatting issue detected. Replace the line break as suggested by prettier/prettier.
[error] 85-85: Prettier formatting issue detected. Replace the line break as suggested by prettier/prettier.
[error] 92-92: Prettier formatting issue detected. Replace the line break as suggested by prettier/prettier.
components/YouTubeEmbed.tsx
[warning] 15-15: Missing JSDoc comment.
🔇 Additional comments (4)
pages/roadmap.tsx (2)
885-952: Thumbnail-based video facade implementation looks good.The click-to-play pattern with thumbnail, gradient overlay, and centered play button aligns well with the PR objectives to defer YouTube player initialization until user interaction. The
stopPropagation()on channel links correctly prevents triggering the video modal when clicking the channel header.
860-877: RoadmapColumn titles are empty strings.All three
RoadmapColumncomponents have emptytitle=''props. The component explicitly renders titles in an h3 heading element, and the descriptions ("Our top priority...", "Our next priority...", "Not a priority...") strongly suggest these should be labeled "Now", "Later", and "Future" respectively.Verify whether these empty titles are intentional or if they should contain the original labels, as the component design includes dedicated heading rendering for titles.
fix-quotes.js (1)
1-21: This utility script should not be committed to the repository.This appears to be a one-time script used during development to normalize quote styles in
roadmap.tsx. Committing it would:
- Add clutter to the codebase
- Risk accidental re-execution
- Create confusion about its purpose
If quote style consistency is needed project-wide, configure ESLint/Prettier rules instead. Remove this file from the PR before merging.
If you need to run this transformation, execute it locally and commit only the resulting changes to
roadmap.tsx, not the script itself.⛔ Skipped due to learnings
Learnt from: akshatnema Repo: asyncapi/website PR: 3378 File: scripts/markdown/check-markdown.js:1-1 Timestamp: 2024-11-25T18:34:51.303Z Learning: When reviewing `scripts/markdown/check-markdown.js`, optimizations should be addressed in separate issues and not included in the current pull request.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.Learnt from: akshatnema Repo: asyncapi/website PR: 3136 File: scripts/tools/combine-tools.js:122-125 Timestamp: 2024-11-01T12:49:32.805Z Learning: In `scripts/tools/combine-tools.js`, the existing URL parsing logic for `repoUrl` without additional error handling is acceptable and does not require changes.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.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.components/YouTubeEmbed.tsx (1)
21-30: The suggested change toyoutube-nocookie.com/iframe_apiwould not work—the IFrame API endpoint only exists atyoutube.com.The
youtube-nocookie.comdomain only provides privacy-enhanced support for plain iframe embeds (e.g.,https://www.youtube-nocookie.com/embed/VIDEO_ID). The IFrame Player API script has no documented endpoint atyoutube-nocookie.comand must load fromyoutube.com. Changing the API URL as proposed would break the functionality.If the PR prioritizes privacy, consider: (1) using plain
youtube-nocookie.comembeds instead of the API if full JS control is not needed, or (2) implementing a consent-based approach that lazy-loads the API only after user consent.Likely an incorrect or invalid review comment.
Anexus5919
force-pushed
the
feature/refine-video-facade
branch
2 times, most recently
from
e1786f8 to
292ca8a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4939 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 796 796
Branches 146 146
=========================================
Hits 796 796 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @pages/roadmap.tsx:
- Around line 686-693: The clickable thumbnail div currently only handles
onClick and is not keyboard accessible; update the element containing the
onClick handler (the div that calls setCurrentVideoId('u83V2gIUGHU') and
setVideoModalOpen(true)) to include role="button", tabIndex={0}, an appropriate
aria-label, and an onKeyDown handler that triggers the same actions when Enter
or Space is pressed (call setCurrentVideoId and setVideoModalOpen). Ensure the
onKeyDown logic prevents default for Space and mirrors the onClick behavior so
keyboard users can focus and activate the thumbnail.
🧹 Nitpick comments (2)
pages/roadmap.tsx (2)
748-752: Modal integration is functional.The conditional rendering and prop passing are correct. The
appendSrc='?start=86'works because theYouTubeEmbedcomponent extracts the start time via regex. However, consider defining the video ID and start time as constants at the top of the component for maintainability.✨ Optional: Extract video configuration as constants
export default function RoadmapPage() { const description: string = 'Long-term vision and plans for the AsyncAPI Initiative.'; const image: string = '/img/social/website-card.jpg'; + const ROADMAP_VIDEO_ID = 'u83V2gIUGHU'; + const ROADMAP_VIDEO_START = 86; const [videoModalOpen, setVideoModalOpen] = useState(false); - const [currentVideoId, setCurrentVideoId] = useState('');Then use these constants in the onClick and Modal:
onClick={() => { - setCurrentVideoId('u83V2gIUGHU'); setVideoModalOpen(true); }}<Modal title='AsyncAPI Video' onModalClose={() => setVideoModalOpen(false)}> - <YouTubeEmbed id={currentVideoId} appendSrc='?start=86' /> + <YouTubeEmbed id={ROADMAP_VIDEO_ID} appendSrc={`start=${ROADMAP_VIDEO_START}`} /> </Modal>This also removes the need for
currentVideoIdstate since there's only one video.
695-699: Consider adding image error handling and lazy loading.The
maxresdefault.jpgthumbnail may not always be available for all YouTube videos. While it likely exists for this specific video, adding a fallback improves robustness. Additionally,loading="lazy"can improve initial page load.✨ Optional: Add fallback and lazy loading
<img src='https://img.youtube.com/vi/u83V2gIUGHU/maxresdefault.jpg' alt='AsyncAPI SIG meeting 51' className='w-full h-full object-cover' + loading='lazy' + onError={(e) => { + e.currentTarget.src = 'https://img.youtube.com/vi/u83V2gIUGHU/hqdefault.jpg'; + }} />
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/YouTubeEmbed.tsxpages/roadmap.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- components/YouTubeEmbed.tsx
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: asyncapi-bot
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
📚 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/roadmap.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/roadmap.tsx
📚 Learning: 2024-11-25T18:34:51.303Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:34:51.303Z
Learning: When reviewing `scripts/markdown/check-markdown.js`, optimizations should be addressed in separate issues and not included in the current pull request.
Applied to files:
pages/roadmap.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:
pages/roadmap.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 a project-specific requirement enforced by the linting rules and build setup, even though it differs from typical Node.js behavior.
Applied to files:
pages/roadmap.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 JavaScript test files within the AsyncAPI website project, TypeScript file imports must include the .ts extension to avoid lint errors, even though the files being imported are JavaScript files.
Applied to files:
pages/roadmap.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:
pages/roadmap.tsx
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.
Applied to files:
pages/roadmap.tsx
🧬 Code graph analysis (1)
pages/roadmap.tsx (3)
components/Modal.tsx (1)
Modal(15-77)components/YouTubeEmbed.tsx (1)
YouTubeEmbed(15-87)components/Warning.tsx (1)
Warning(18-36)
⏰ 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 (3)
pages/roadmap.tsx (3)
491-492: LGTM!State hooks are properly initialized. Using an empty string for
currentVideoIdis appropriate since it's only used when the modal is open.
709-732: Channel links implementation looks correct.The
stopPropagationpattern properly prevents the modal from opening when users click the channel avatar or name. Security attributes (rel="noopener noreferrer") are correctly applied for external links.
755-761: LGTM!The Warning component usage and text adjustment are appropriate.
Anexus5919
force-pushed
the
feature/refine-video-facade
branch
from
292ca8a to
a06fe9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @components/YouTubeEmbed.tsx:
- Around line 19-31: pendingPlayers and isAPIReady are recreated on every render
causing race conditions and the global callback assignment runs during render;
change pendingPlayers and isAPIReady to refs (e.g., pendingPlayersRef =
useRef<(() => void)[]>([]) and isAPIReadyRef = useRef(false)), move the
window.onYouTubeIframeAPIReady wiring (capture originalCallback into a ref if
needed) into a useEffect that runs once, and update the player initialization
useEffect to read/write pendingPlayersRef.current and isAPIReadyRef.current
instead of the local variables so the queue and ready flag persist across
renders and the global callback uses the current refs.
🧹 Nitpick comments (1)
components/YouTubeEmbed.tsx (1)
92-93: Missing dependencyappendSrcand eslint-disable lacks explanation.The
appendSrcprop is used insidecreatePlayer(lines 64-69) but is missing from the dependency array. IfappendSrcchanges while the modal is open, the player won't seek to the new start time.Additionally, per project conventions, eslint-disable comments should include an explanatory note. Based on learnings from this repository.
Suggested fix
- // eslint-disable-next-line react-hooks/exhaustive-deps - }, [id]); + // eslint-disable-next-line react-hooks/exhaustive-deps -- appendSrc is intentionally omitted; start time is only applied on initial player creation + }, [id]);Alternatively, if you want the player to respond to
appendSrcchanges:- // eslint-disable-next-line react-hooks/exhaustive-deps - }, [id]); + }, [id, appendSrc]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/YouTubeEmbed.tsxpages/roadmap.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- pages/roadmap.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: asyncapi-bot
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.
Applied to files:
components/YouTubeEmbed.tsx
⏰ 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). (5)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - macos-latest
Anexus5919
force-pushed
the
feature/refine-video-facade
branch
from
a06fe9e to
588b267
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @components/YouTubeEmbed.tsx:
- Around line 22-40: The useEffect in YouTubeEmbed overwrites
window.onYouTubeIframeAPIReady per instance which causes a race where cleanup
can restore an outdated callback and drop other instances' handlers; replace the
per-instance overwrite with a shared module-level manager: create a singleton
registry that exposes addReadyListener/removeReadyListener and an initOnce that
sets window.onYouTubeIframeAPIReady to call all registered listeners and set
isAPIReadyRef-like state; update the useEffect to register/unregister this
instance’s pendingPlayersRef callback via addReadyListener/removeReadyListener
and stop directly mutating window.onYouTubeIframeAPIReady so multiple
YouTubeEmbed mounts don’t clobber each other.
🧹 Nitpick comments (5)
components/YouTubeEmbed.tsx (4)
3-8: Consider using a more specific type forwindow.YT.Using
anyforwindow.YTloses type safety. While the YouTube IFrame API doesn't have official TypeScript types, you could define a minimal interface for the parts you use, or consider using@types/youtubefrom DefinitelyTyped.💡 Suggested type definition
declare global { interface Window { onYouTubeIframeAPIReady: () => void; - YT: any; + YT: { + Player: new ( + element: HTMLElement | string, + options: { + height?: string | number; + width?: string | number; + videoId?: string; + playerVars?: Record<string, number | string>; + events?: { + onReady?: (event: { target: { seekTo: (seconds: number, allowSeekAhead: boolean) => void; playVideo: () => void } }) => void; + }; + } + ) => { destroy: () => void }; + }; } }
42-52: Consider adding error handling for script load failure.If the YouTube IFrame API script fails to load (network error, blocked by content policy, etc.), the component will silently fail without any feedback. Consider adding an
onerrorhandler.💡 Suggested improvement
useEffect(() => { if (!window.YT) { const tag = document.createElement('script'); tag.src = 'https://www.youtube.com/iframe_api'; + tag.onerror = () => { + console.error('Failed to load YouTube IFrame API'); + }; const firstScriptTag = document.getElementsByTagName('script')[0]; firstScriptTag.parentNode?.insertBefore(tag, firstScriptTag); } }, []);
71-81: RedundantplayVideo()call whenautoplay: 1is already set.With
autoplay: 1inplayerVars(line 69), callingevent.target.playVideo()on theonReadyevent is redundant. The video will already begin playing automatically. Consider removing one or the other for clarity.💡 Suggested simplification
events: { onReady: (event: any) => { if (appendSrc && appendSrc.includes('start=')) { const startMatch = appendSrc.match(/start=(\d+)/); if (startMatch && startMatch[1]) { event.target.seekTo(parseInt(startMatch[1], 10), true); } } - event.target.playVideo(); } }
101-102: Add an explanatory note to the eslint-disable comment.Based on learnings from this repository, when intentionally omitting dependencies in React hooks, the comment should include an explanation. Here,
appendSrcis omitted to avoid re-creating the player when only the start time changes.💡 Suggested improvement
- // eslint-disable-next-line react-hooks/exhaustive-deps + // eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally omitting `appendSrc` to avoid re-creating the player; start time is handled in onReady }, [id]);pages/roadmap.tsx (1)
708-712: Consider using Next.jsImagecomponent for the thumbnail.Using a standard
<img>tag for the YouTube thumbnail bypasses Next.js image optimization. While external images require configuration innext.config.js, consider using theImagecomponent for better performance (lazy loading, responsive sizing).💡 Suggested change
- <img - src='https://img.youtube.com/vi/u83V2gIUGHU/maxresdefault.jpg' - alt='AsyncAPI SIG meeting 51' - className='w-full h-full object-cover' - /> + <Image + src='https://img.youtube.com/vi/u83V2gIUGHU/maxresdefault.jpg' + alt='AsyncAPI SIG meeting 51' + fill + className='object-cover' + unoptimized + />Note: You'll need to add
img.youtube.comto theimages.remotePatternsinnext.config.js, or useunoptimizedprop as shown above.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/YouTubeEmbed.tsxpages/roadmap.tsx
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: asyncapi-bot
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.
Applied to files:
components/YouTubeEmbed.tsxpages/roadmap.tsx
📚 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/roadmap.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/roadmap.tsx
📚 Learning: 2024-11-25T18:34:51.303Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:34:51.303Z
Learning: When reviewing `scripts/markdown/check-markdown.js`, optimizations should be addressed in separate issues and not included in the current pull request.
Applied to files:
pages/roadmap.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:
pages/roadmap.tsx
📚 Learning: 2025-01-08T15:15:00.759Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3557
File: scripts/markdown/check-editlinks.js:58-59
Timestamp: 2025-01-08T15:15:00.759Z
Learning: In the AsyncAPI codebase, batch processing operations (like in the Dashboard script and check-editlinks.js) follow a sequential pattern using await in loops, which is the preferred approach for maintaining consistency across the codebase.
Applied to files:
pages/roadmap.tsx
📚 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/roadmap.tsx
🧬 Code graph analysis (1)
pages/roadmap.tsx (3)
components/Modal.tsx (1)
Modal(15-77)components/YouTubeEmbed.tsx (1)
YouTubeEmbed(15-109)components/Warning.tsx (1)
Warning(18-36)
⏰ 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 (7)
components/YouTubeEmbed.tsx (1)
104-108: LGTM!The responsive container using the padding-top trick for 16:9 aspect ratio (56.25%) is a solid approach for maintaining video proportions across screen sizes.
pages/roadmap.tsx (6)
2-16: LGTM!Imports are properly organized, adding
useStatefrom React and the newModalandYouTubeEmbedcomponents.
61-65: Good addition ofrel='noopener noreferrer'.Adding this attribute to external links with
target='_blank'is a security best practice to prevent reverse tabnabbing attacks.
495-496: LGTM!State hooks are appropriately initialized for managing the modal visibility and current video ID.
761-765: LGTM with minor suggestion.The modal integration works correctly. The
appendSrc='?start=86'will match the regex inYouTubeEmbed. Consider using a more specific modal title like "AsyncAPI SIG Meeting 51" for better context.
768-774: LGTM!The Warning component usage and description text are appropriate.
690-746: No hydration issue with this pattern.The nested anchor tags inside
div[role="button"]do not cause hydration issues. The learnings from this repository specifically address hydration problems with nested<a>tags inside Next.js<Link>components (e.g.,<Link><a>...</a></Link>), which create invalid HTML structures. This code uses a semanticdiv[role="button"]containing independent anchor tags—a valid HTML pattern. The implementation properly manages event propagation withstopPropagation()and pointer events, allowing the nested links to function correctly without interfering with the parent button's click handler.
| useEffect(() => { | ||
| const originalCallback = window.onYouTubeIframeAPIReady; | ||
|
|
||
| window.onYouTubeIframeAPIReady = () => { | ||
| isAPIReadyRef.current = true; | ||
| originalCallback?.(); | ||
| pendingPlayersRef.current.forEach((fn) => fn()); | ||
| pendingPlayersRef.current = []; | ||
| }; | ||
|
|
||
| // If API already loaded before this effect ran | ||
| if (window.YT && window.YT.Player) { | ||
| isAPIReadyRef.current = true; | ||
| } | ||
|
|
||
| return () => { | ||
| window.onYouTubeIframeAPIReady = originalCallback; | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
Race condition: API callback restoration may break other instances.
When multiple YouTubeEmbed components mount concurrently, each overwrites window.onYouTubeIframeAPIReady. The cleanup restores originalCallback, but if instance B mounts after instance A, instance A's cleanup will restore the callback that existed before A (not B's callback), potentially causing B's pending players to never initialize.
Consider using a shared singleton pattern or event-based approach instead of overwriting the global callback per instance.
🔧 Suggested fix using a module-level singleton
+// Module-level singleton for API readiness
+const pendingCallbacks: (() => void)[] = [];
+let isYTAPIReady = false;
+let isCallbackRegistered = false;
+
+function registerYTCallback(callback: () => void) {
+ if (isYTAPIReady) {
+ callback();
+ } else {
+ pendingCallbacks.push(callback);
+ if (!isCallbackRegistered) {
+ isCallbackRegistered = true;
+ const original = window.onYouTubeIframeAPIReady;
+ window.onYouTubeIframeAPIReady = () => {
+ isYTAPIReady = true;
+ original?.();
+ pendingCallbacks.forEach((fn) => fn());
+ pendingCallbacks.length = 0;
+ };
+ }
+ }
+}
+
export default function YouTubeEmbed({ id, appendSrc = '' }: YouTubeEmbedProps) {
const playerId = useId();
const containerRef = useRef<HTMLDivElement>(null);
const playerRef = useRef<any>(null);
- const pendingPlayersRef = useRef<(() => void)[]>([]);
- const isAPIReadyRef = useRef(false);
-
- useEffect(() => {
- const originalCallback = window.onYouTubeIframeAPIReady;
-
- window.onYouTubeIframeAPIReady = () => {
- isAPIReadyRef.current = true;
- originalCallback?.();
- pendingPlayersRef.current.forEach((fn) => fn());
- pendingPlayersRef.current = [];
- };
-
- // If API already loaded before this effect ran
- if (window.YT && window.YT.Player) {
- isAPIReadyRef.current = true;
- }
-
- return () => {
- window.onYouTubeIframeAPIReady = originalCallback;
- };
- }, []);🤖 Prompt for AI Agents
In @components/YouTubeEmbed.tsx around lines 22 - 40, The useEffect in
YouTubeEmbed overwrites window.onYouTubeIframeAPIReady per instance which causes
a race where cleanup can restore an outdated callback and drop other instances'
handlers; replace the per-instance overwrite with a shared module-level manager:
create a singleton registry that exposes addReadyListener/removeReadyListener
and an initOnce that sets window.onYouTubeIframeAPIReady to call all registered
listeners and set isAPIReadyRef-like state; update the useEffect to
register/unregister this instance’s pendingPlayersRef callback via
addReadyListener/removeReadyListener and stop directly mutating
window.onYouTubeIframeAPIReady so multiple YouTubeEmbed mounts don’t clobber
each other.
2 participants
Closes #4891
Summary
This PR introduces a refined video facade implementation to enhance the video playback experience on the Roadmap page. It optimizes the intro video with a custom, high-performance facade.
Issue
Previously, the video embed on the Roadmap page lacked a unified, optimized loading strategy. Standard iframes can negatively impact initial page load performance. There was also a need for consistent styling and behavior.
Expected Result
rel=0,modestbranding=1).Environment Tested
Before
1. On pausing
2. On completion
After
Vision.Roadmap._.AsyncAPI.Initiative.for.event-driven.APIs.-.Google.Chrome.2026-01-13.00-00-53.mp4
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.