Open
Conversation
dosubot
bot
added
the
size:XL
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
WalkthroughAdds a “Learn More” system: new UI components 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
mcpjam-inspector/client/src/components/sidebar/__tests__/nav-main.test.tsx (1)
15-15: MakeuseSidebarconfigurable per test.The new wrapper logic is gated on
open, but this mock freezes the suite in the expanded branch. A smallmockUseSidebarhelper would let you cover the collapsed-sidebar path too and keep that gate honest.Based on learnings, All changes should include tests using Vitest. Run tests with
npm run test,test:watch, ortest:coverage.Also applies to: 83-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/sidebar/__tests__/nav-main.test.tsx` at line 15, The suite currently hardcodes the useSidebar mock to always return open: true which forces the wrapper into the expanded branch; introduce a small test helper (e.g., mockUseSidebar) that can be configured per-test to return either { open: true } or { open: false } and replace the inline mock in nav-main.test.tsx with calls to mockUseSidebar in each test; update or add a test that asserts the collapsed-sidebar behavior (the branch when open: false) so the wrapper gating logic in the component (the prop/state checked by the wrapper) is covered and both expanded and collapsed paths are exercised by Vitest.mcpjam-inspector/client/src/components/sidebar/sidebar-workspace-selector.tsx (1)
127-221: Extract the trigger once and only branch at the wrapper.Both paths now carry the same
SidebarMenuButtonsubtree. Hoisting that markup into a singletriggerconstant will keep avatar and layout tweaks from drifting between the learn-more and fallback paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/sidebar/sidebar-workspace-selector.tsx` around lines 127 - 221, The SidebarMenuButton subtree (including DropdownMenuTrigger, WorkspaceIconBadge, Avatar list, ChevronsUpDown, etc.) is duplicated; extract it into a single `trigger` constant (or render function) and then conditionally wrap that `trigger` with LearnMoreHoverCard only when onLearnMoreExpand is truthy; update locations referencing SidebarMenuButton, DropdownMenuTrigger, WorkspaceIconBadge, Avatar/AvatarImage/AvatarFallback, ChevronsUpDown and LearnMoreHoverCard so the JSX for the button/avatars lives once and the branching only happens at the wrapper.mcpjam-inspector/client/src/components/learn-more/LearnMoreHoverCard.tsx (1)
10-29: Blob URLs persist indefinitely—consider memory implications.This preloading approach trades memory for instant playback. Since
URL.revokeObjectURL()is never called, these blob URLs remain in memory for the session lifetime. For the handful of preview videos currently defined, this is acceptable. Should the content catalog grow substantially, consider a bounded LRU cache or cleanup on navigation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/learn-more/LearnMoreHoverCard.tsx` around lines 10 - 29, Preloading preview videos into blobCache using URL.createObjectURL without ever calling URL.revokeObjectURL causes blob URLs to persist and leak memory; update the preloading logic around blobCache and the fetch promise to call URL.revokeObjectURL when a blob URL is no longer needed and implement a bounded eviction strategy (e.g., LRU) for blobCache keyed by previewVideoUrl, or at minimum add a cleanup hook that revokes all entries on navigation/unmount (tie to component lifecycle or window beforeunload), ensuring you reference blobCache, learnMoreContent, and the createObjectURL usage when modifying the code.mcpjam-inspector/client/src/components/learn-more/LearnMoreExpandedPanel.tsx (1)
213-218: Theas anycast sidesteps a framer-motion typing limitation.This is a known friction point with nested
transitionoverrides in framer-motion's TypeScript definitions. Acceptable as-is, though a future library update may resolve it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/learn-more/LearnMoreExpandedPanel.tsx` around lines 213 - 218, The exit transition object currently uses an unsafe cast ("as any") to bypass framer-motion typings; replace that with a properly typed Transition by importing Transition from 'framer-motion' and typing the object (e.g., const exitTransition: Transition = { opacity: 0, scale: 0.97, transition: { duration: 0.15, ease: EASING } } or move the nested transition into an object typed as Transition), then pass exit={exitTransition} and keep transition={{ duration: 0.35, ease: EASING }} to remove the "as any" cast in LearnMoreExpandedPanel (reference: exit prop, transition prop, EASING).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@mcpjam-inspector/client/src/components/learn-more/LearnMoreExpandedPanel.tsx`:
- Around line 15-129: VideoThumbnail duplicates logic found in
ModalVideoThumbnail (play state, youtubeId parsing, thumbnailSrc precedence),
causing drift; extract a single shared component (e.g., export function
VideoThumbnail({ entry }: VideoThumbnailProps)) placed in a common folder and
have both LearnMoreExpandedPanel.tsx and LearnMoreModal.tsx import it. Ensure
the unified implementation uses the desired thumbnail precedence (decide whether
entry.videoThumbnail or YouTube thumbnail wins and keep that logic consistent in
thumbnailSrc), preserve props/behavior for isMP4, isYouTube, youtubeId, playing
state, videoRef, and the play button/Watch on YouTube anchor so both callers
work unchanged. After extraction, remove the local
ModalVideoThumbnail/VideoThumbnail duplicates and replace them with the shared
VideoThumbnail import.
- Around line 131-147: The expanded panel (LearnMoreExpandedPanel) lacks focus
management: when tabId becomes truthy move focus into the panel, trap focus
while open, and restore focus to the trigger on close. In the effect that
watches [tabId, onClose] use panelRef to save document.activeElement before
opening, set panelRef.current.focus() (ensure the rendered motion.div has
tabIndex={-1}), and on cleanup restore focus to the saved element; for full
accessibility replace this manual handling with a focus-trap (e.g.,
focus-trap-react) or the existing Dialog primitive used elsewhere to enforce
focus trapping and return-focus behavior. Ensure handlers (panelRef, onClose)
and the cleanup logic are updated in LearnMoreExpandedPanel to implement these
steps.
In `@mcpjam-inspector/client/src/components/learn-more/LearnMoreModal.tsx`:
- Around line 63-67: The thumbnail selection in LearnMoreModal (variable
thumbnailSrc using isMP4, isYouTube, youtubeId, and entry.videoThumbnail)
currently prefers YouTube thumbnails over entry.videoThumbnail, which is
opposite LearnMoreExpandedPanel; make behavior consistent by changing the
precedence so entry.videoThumbnail is used first (unless isMP4), and only fall
back to the YouTube URL when entry.videoThumbnail is falsy and isYouTube &&
youtubeId; if the inverse precedence was intentional, add a short comment in
LearnMoreModal explaining the rationale and reference LearnMoreExpandedPanel for
clarity.
- Around line 17-19: The ModalVideoThumbnail component declares an unused ref
(videoRef) via useRef but never reads it; remove the unused videoRef declaration
(the const videoRef = useRef<HTMLVideoElement>(null) line) and delete any
ref={videoRef} prop on the <video> element if present so ModalVideoThumbnail
only keeps the playing/setPlaying state it actually uses.
In `@mcpjam-inspector/client/src/hooks/use-learn-more.ts`:
- Around line 8-10: The hasLearnMoreContent callback currently uses the `in`
operator which checks the prototype chain; change its property check to only own
properties by replacing the `in` usage with a direct own-property test (e.g.,
`Object.hasOwn(learnMoreContent, tabId)` or
`Object.prototype.hasOwn.call(learnMoreContent, tabId)`) inside the
`hasLearnMoreContent` function defined with `useCallback`, ensuring you
reference `learnMoreContent` and `tabId` as before.
---
Nitpick comments:
In
`@mcpjam-inspector/client/src/components/learn-more/LearnMoreExpandedPanel.tsx`:
- Around line 213-218: The exit transition object currently uses an unsafe cast
("as any") to bypass framer-motion typings; replace that with a properly typed
Transition by importing Transition from 'framer-motion' and typing the object
(e.g., const exitTransition: Transition = { opacity: 0, scale: 0.97, transition:
{ duration: 0.15, ease: EASING } } or move the nested transition into an object
typed as Transition), then pass exit={exitTransition} and keep transition={{
duration: 0.35, ease: EASING }} to remove the "as any" cast in
LearnMoreExpandedPanel (reference: exit prop, transition prop, EASING).
In `@mcpjam-inspector/client/src/components/learn-more/LearnMoreHoverCard.tsx`:
- Around line 10-29: Preloading preview videos into blobCache using
URL.createObjectURL without ever calling URL.revokeObjectURL causes blob URLs to
persist and leak memory; update the preloading logic around blobCache and the
fetch promise to call URL.revokeObjectURL when a blob URL is no longer needed
and implement a bounded eviction strategy (e.g., LRU) for blobCache keyed by
previewVideoUrl, or at minimum add a cleanup hook that revokes all entries on
navigation/unmount (tie to component lifecycle or window beforeunload), ensuring
you reference blobCache, learnMoreContent, and the createObjectURL usage when
modifying the code.
In `@mcpjam-inspector/client/src/components/sidebar/__tests__/nav-main.test.tsx`:
- Line 15: The suite currently hardcodes the useSidebar mock to always return
open: true which forces the wrapper into the expanded branch; introduce a small
test helper (e.g., mockUseSidebar) that can be configured per-test to return
either { open: true } or { open: false } and replace the inline mock in
nav-main.test.tsx with calls to mockUseSidebar in each test; update or add a
test that asserts the collapsed-sidebar behavior (the branch when open: false)
so the wrapper gating logic in the component (the prop/state checked by the
wrapper) is covered and both expanded and collapsed paths are exercised by
Vitest.
In
`@mcpjam-inspector/client/src/components/sidebar/sidebar-workspace-selector.tsx`:
- Around line 127-221: The SidebarMenuButton subtree (including
DropdownMenuTrigger, WorkspaceIconBadge, Avatar list, ChevronsUpDown, etc.) is
duplicated; extract it into a single `trigger` constant (or render function) and
then conditionally wrap that `trigger` with LearnMoreHoverCard only when
onLearnMoreExpand is truthy; update locations referencing SidebarMenuButton,
DropdownMenuTrigger, WorkspaceIconBadge, Avatar/AvatarImage/AvatarFallback,
ChevronsUpDown and LearnMoreHoverCard so the JSX for the button/avatars lives
once and the branching only happens at the wrapper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdde2ada-715d-49e7-96fb-2dc01437a72c
⛔ Files ignored due to path filters (1)
mcpjam-inspector/client/public/tool-vid-march.mp4is excluded by!**/*.mp4
📒 Files selected for processing (11)
mcpjam-inspector/client/src/components/learn-more/LearnMoreExpandedPanel.tsxmcpjam-inspector/client/src/components/learn-more/LearnMoreHoverCard.tsxmcpjam-inspector/client/src/components/learn-more/LearnMoreModal.tsxmcpjam-inspector/client/src/components/learn-more/__tests__/LearnMoreExpandedPanel.test.tsxmcpjam-inspector/client/src/components/mcp-sidebar.tsxmcpjam-inspector/client/src/components/sidebar/__tests__/nav-main.test.tsxmcpjam-inspector/client/src/components/sidebar/__tests__/sidebar-workspace-selector.test.tsxmcpjam-inspector/client/src/components/sidebar/nav-main.tsxmcpjam-inspector/client/src/components/sidebar/sidebar-workspace-selector.tsxmcpjam-inspector/client/src/hooks/use-learn-more.tsmcpjam-inspector/client/src/lib/learn-more-content.ts
Comment on lines
+15
to
+129
| function VideoThumbnail({ entry }: { entry: { title: string; videoUrl: string; videoThumbnail?: string } }) { | ||
| const [playing, setPlaying] = useState(false); | ||
| const videoRef = useRef<HTMLVideoElement>(null); | ||
|
|
||
| const isMP4 = entry.videoUrl?.endsWith(".mp4"); | ||
| const isYouTube = entry.videoUrl?.includes("youtube.com/embed/"); | ||
| const youtubeId = isYouTube ? entry.videoUrl.split("/embed/")[1]?.split("?")[0] : null; | ||
| const hasVideo = !!(entry.videoUrl); | ||
|
|
||
| if (!hasVideo) { | ||
| return ( | ||
| <div className="aspect-video w-full bg-muted flex items-center justify-center rounded-lg"> | ||
| <p className="text-muted-foreground text-sm">Video coming soon</p> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| // Playing state: show the actual video/iframe | ||
| if (playing) { | ||
| return ( | ||
| <div className="aspect-video w-full overflow-hidden rounded-lg bg-black"> | ||
| {isMP4 ? ( | ||
| <video | ||
| ref={videoRef} | ||
| src={entry.videoUrl} | ||
| className="h-full w-full" | ||
| autoPlay | ||
| loop | ||
| muted | ||
| playsInline | ||
| preload="auto" | ||
| controls | ||
| title={`${entry.title} video`} | ||
| /> | ||
| ) : ( | ||
| <iframe | ||
| src={`${entry.videoUrl}${entry.videoUrl.includes("?") ? "&" : "?"}autoplay=1`} | ||
| className="h-full w-full" | ||
| allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture" | ||
| allowFullScreen | ||
| title={`${entry.title} video`} | ||
| /> | ||
| )} | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| // Thumbnail state (Notion-style): dark overlay + title + play button + Watch on YouTube | ||
| const thumbnailSrc = entry.videoThumbnail | ||
| ? entry.videoThumbnail | ||
| : isMP4 | ||
| ? undefined | ||
| : isYouTube && youtubeId | ||
| ? `https://img.youtube.com/vi/${youtubeId}/hqdefault.jpg` | ||
| : undefined; | ||
|
|
||
| return ( | ||
| <div className="relative aspect-video w-full overflow-hidden rounded-lg bg-neutral-900 group"> | ||
| {/* Background image / video poster */} | ||
| {thumbnailSrc ? ( | ||
| <img | ||
| src={thumbnailSrc} | ||
| alt={`${entry.title} preview`} | ||
| className="h-full w-full object-cover" | ||
| /> | ||
| ) : isMP4 ? ( | ||
| <video | ||
| src={entry.videoUrl} | ||
| className="h-full w-full object-cover" | ||
| muted | ||
| playsInline | ||
| preload="metadata" | ||
| /> | ||
| ) : null} | ||
|
|
||
| {/* Dark gradient overlay */} | ||
| <div className="pointer-events-none absolute inset-0 bg-gradient-to-t from-black/70 via-black/40 to-black/30 group-hover:from-black/75 group-hover:via-black/45 group-hover:to-black/35 transition-colors" /> | ||
|
|
||
| {/* Title overlay (top-left) */} | ||
| <div className="pointer-events-none absolute top-4 left-5"> | ||
| <p className="text-white text-lg font-semibold drop-shadow-md"> | ||
| {entry.title} | ||
| </p> | ||
| <p className="text-white/70 text-sm">MCPJam Inspector</p> | ||
| </div> | ||
|
|
||
| {/* Centered play button */} | ||
| <div className="pointer-events-none absolute inset-0 flex items-center justify-center"> | ||
| <div className="rounded-full bg-white/90 group-hover:bg-white p-4 shadow-lg transition-colors"> | ||
| <Play className="h-6 w-6 text-black fill-black" /> | ||
| </div> | ||
| </div> | ||
|
|
||
| <button | ||
| type="button" | ||
| onClick={() => setPlaying(true)} | ||
| className="absolute inset-0 z-10 rounded-lg focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2" | ||
| aria-label={`Play ${entry.title} video`} | ||
| /> | ||
|
|
||
| {/* Watch on YouTube badge (bottom-right) — only for YouTube videos */} | ||
| {isYouTube && youtubeId && ( | ||
| <a | ||
| href={`https://www.youtube.com/watch?v=${youtubeId}`} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| onClick={(e) => e.stopPropagation()} | ||
| className="absolute bottom-3 right-4 z-20 flex items-center gap-1.5 bg-black/70 hover:bg-black/90 text-white text-xs font-medium px-3 py-1.5 rounded transition-colors" | ||
| > | ||
| Watch on <span className="font-bold">YouTube</span> | ||
| </a> | ||
| )} | ||
| </div> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract shared VideoThumbnail to eliminate duplication.
This component is nearly identical to ModalVideoThumbnail in LearnMoreModal.tsx. Both share the same thumbnail logic, play state management, and YouTube handling—yet differ subtly in thumbnailSrc precedence (this version checks entry.videoThumbnail before YouTube; the modal does the opposite). Consolidating into a single shared component would reduce maintenance burden and ensure consistent behavior.
♻️ Suggested approach
Create a shared component in a common location:
// e.g., `@/components/learn-more/VideoThumbnail.tsx`
export function VideoThumbnail({ entry }: VideoThumbnailProps) {
// unified implementation
}Then import it in both LearnMoreExpandedPanel.tsx and LearnMoreModal.tsx.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/learn-more/LearnMoreExpandedPanel.tsx`
around lines 15 - 129, VideoThumbnail duplicates logic found in
ModalVideoThumbnail (play state, youtubeId parsing, thumbnailSrc precedence),
causing drift; extract a single shared component (e.g., export function
VideoThumbnail({ entry }: VideoThumbnailProps)) placed in a common folder and
have both LearnMoreExpandedPanel.tsx and LearnMoreModal.tsx import it. Ensure
the unified implementation uses the desired thumbnail precedence (decide whether
entry.videoThumbnail or YouTube thumbnail wins and keep that logic consistent in
thumbnailSrc), preserve props/behavior for isMP4, isYouTube, youtubeId, playing
state, videoRef, and the play button/Watch on YouTube anchor so both callers
work unchanged. After extraction, remove the local
ModalVideoThumbnail/VideoThumbnail duplicates and replace them with the shared
VideoThumbnail import.
Comment on lines
+131
to
+147
| export function LearnMoreExpandedPanel({ | ||
| tabId, | ||
| sourceRect, | ||
| onClose, | ||
| }: LearnMoreExpandedPanelProps) { | ||
| const entry = tabId ? learnMoreContent[tabId] : null; | ||
| const panelRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| // Close on Escape | ||
| useEffect(() => { | ||
| if (!tabId) return; | ||
| const handleKey = (e: KeyboardEvent) => { | ||
| if (e.key === "Escape") onClose(); | ||
| }; | ||
| document.addEventListener("keydown", handleKey); | ||
| return () => document.removeEventListener("keydown", handleKey); | ||
| }, [tabId, onClose]); |
Contributor
There was a problem hiding this comment.
Accessibility: Panel lacks focus management.
Per WCAG modal dialog patterns, this expanded panel should:
- Move focus into the panel upon opening
- Trap focus within while open
- Return focus to the trigger element on close
Currently, only Escape-to-close is implemented. Consider leveraging a focus-trap library or the <Dialog> primitive (as used in LearnMoreModal) to handle these automatically.
🔧 Minimal focus-on-open fix
useEffect(() => {
if (!tabId) return;
+ // Move focus into panel on open
+ panelRef.current?.focus();
const handleKey = (e: KeyboardEvent) => {
if (e.key === "Escape") onClose();
};
document.addEventListener("keydown", handleKey);
return () => document.removeEventListener("keydown", handleKey);
}, [tabId, onClose]);Add tabIndex={-1} to the panel's motion.div to make it focusable. For full compliance, integrate a focus-trap solution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/learn-more/LearnMoreExpandedPanel.tsx`
around lines 131 - 147, The expanded panel (LearnMoreExpandedPanel) lacks focus
management: when tabId becomes truthy move focus into the panel, trap focus
while open, and restore focus to the trigger on close. In the effect that
watches [tabId, onClose] use panelRef to save document.activeElement before
opening, set panelRef.current.focus() (ensure the rendered motion.div has
tabIndex={-1}), and on cleanup restore focus to the saved element; for full
accessibility replace this manual handling with a focus-trap (e.g.,
focus-trap-react) or the existing Dialog primitive used elsewhere to enforce
focus trapping and return-focus behavior. Ensure handlers (panelRef, onClose)
and the cleanup logic are updated in LearnMoreExpandedPanel to implement these
steps.
mcpjam-inspector/client/src/components/learn-more/LearnMoreModal.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+63
to
+67
| const thumbnailSrc = isMP4 | ||
| ? undefined | ||
| : isYouTube && youtubeId | ||
| ? `https://img.youtube.com/vi/${youtubeId}/hqdefault.jpg` | ||
| : entry.videoThumbnail; |
Contributor
There was a problem hiding this comment.
Thumbnail resolution order differs from LearnMoreExpandedPanel.
Here, YouTube thumbnail takes precedence over entry.videoThumbnail. In LearnMoreExpandedPanel, the order is reversed. If intentional, document why; otherwise, unify for consistent behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/learn-more/LearnMoreModal.tsx` around
lines 63 - 67, The thumbnail selection in LearnMoreModal (variable thumbnailSrc
using isMP4, isYouTube, youtubeId, and entry.videoThumbnail) currently prefers
YouTube thumbnails over entry.videoThumbnail, which is opposite
LearnMoreExpandedPanel; make behavior consistent by changing the precedence so
entry.videoThumbnail is used first (unless isMP4), and only fall back to the
YouTube URL when entry.videoThumbnail is falsy and isYouTube && youtubeId; if
the inverse precedence was intentional, add a short comment in LearnMoreModal
explaining the rationale and reference LearnMoreExpandedPanel for clarity.
Comment on lines
+8
to
+10
| const hasLearnMoreContent = useCallback((tabId: string): boolean => { | ||
| return tabId in learnMoreContent; | ||
| }, []); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript/TypeScript, does the in operator check inherited properties on an object's prototype chain, and what is the recommended way to test only an object's own keys?
💡 Result:
Yes, the in operator checks inherited properties on an object's prototype chain. The recommended way to test only an object's own keys is Object.hasOwn(object, property).
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/in
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwn
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwnProperty
Use Object.hasOwn() to check only own properties.
The in operator walks the prototype chain, so inherited properties like toString return true even though they're not actual entries. This can admit garbage values to the UI.
🔧 Proposed fix
const hasLearnMoreContent = useCallback((tabId: string): boolean => {
- return tabId in learnMoreContent;
+ return Object.hasOwn(learnMoreContent, tabId);
}, []);📝 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.
Suggested change
| const hasLearnMoreContent = useCallback((tabId: string): boolean => { | |
| return tabId in learnMoreContent; | |
| }, []); | |
| const hasLearnMoreContent = useCallback((tabId: string): boolean => { | |
| return Object.hasOwn(learnMoreContent, tabId); | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/hooks/use-learn-more.ts` around lines 8 - 10, The
hasLearnMoreContent callback currently uses the `in` operator which checks the
prototype chain; change its property check to only own properties by replacing
the `in` usage with a direct own-property test (e.g.,
`Object.hasOwn(learnMoreContent, tabId)` or
`Object.prototype.hasOwn.call(learnMoreContent, tabId)`) inside the
`hasLearnMoreContent` function defined with `useCallback`, ensuring you
reference `learnMoreContent` and `tabId` as before.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/components/learn-more/LearnMoreHoverCard.tsx (1)
10-37: Eliminate eager full-video preload and persistent blob URL caching.The implementation preloads all preview videos (~2.5–3 MB aggregate) on first mount and retains their
blob:URLs indefinitely without revocation. This unnecessarily inflates initial network cost and keeps memory allocated longer than needed.Instead, use native video URLs directly with
preload="metadata"and let the browser handle caching. If you later need per-tab lazy loading for performance, add that then—not upfront.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/learn-more/LearnMoreHoverCard.tsx` around lines 10 - 37, The current preloadPreviewVideos function eagerly fetches every entry.previewVideoUrl, stores blob: URLs in blobCache and never revokes them, causing unnecessary network and memory overhead; remove the fetch/blob creation and persistent blobCache logic: delete blobCache and previewsPreloaded usage, replace preloadPreviewVideos with no-op (or remove it) and update any UI that used blobCache or preloadPreviewVideos to use entry.previewVideoUrl directly, and ensure the video element for previews sets preload="metadata" (use the native URL string from learnMoreContent) so the browser handles caching and avoids creating/holding blob URLs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcpjam-inspector/client/src/components/learn-more/LearnMoreHoverCard.tsx`:
- Around line 10-37: The current preloadPreviewVideos function eagerly fetches
every entry.previewVideoUrl, stores blob: URLs in blobCache and never revokes
them, causing unnecessary network and memory overhead; remove the fetch/blob
creation and persistent blobCache logic: delete blobCache and previewsPreloaded
usage, replace preloadPreviewVideos with no-op (or remove it) and update any UI
that used blobCache or preloadPreviewVideos to use entry.previewVideoUrl
directly, and ensure the video element for previews sets preload="metadata" (use
the native URL string from learnMoreContent) so the browser handles caching and
avoids creating/holding blob URLs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccc47c5b-d1af-464c-875a-d175f3da7e1b
📒 Files selected for processing (3)
mcpjam-inspector/client/src/components/learn-more/LearnMoreHoverCard.tsxmcpjam-inspector/client/src/components/learn-more/__tests__/LearnMoreExpandedPanel.test.tsxmcpjam-inspector/client/src/components/mcp-sidebar.tsx
✅ Files skipped from review due to trivial changes (1)
- mcpjam-inspector/client/src/components/learn-more/tests/LearnMoreExpandedPanel.test.tsx
dosubot
bot
added
size:XXL
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/components/sidebar/sidebar-workspace-selector.tsx (1)
127-225: Extract the shared trigger subtree.Only the wrapper changes here, but the
DropdownMenuTrigger/SidebarMenuButtonbody is duplicated almost verbatim in both branches. Hoisting that tree into a localtriggerconstant will keep future copy, layout, and accessibility edits from drifting between the learn-more and non-learn-more paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/sidebar/sidebar-workspace-selector.tsx` around lines 127 - 225, The DropdownMenuTrigger/SidebarMenuButton subtree is duplicated; extract it into a local constant (e.g. const trigger = (<DropdownMenuTrigger asChild>...<SidebarMenuButton>...</SidebarMenuButton></DropdownMenuTrigger>)) that includes WorkspaceIconBadge, workspaceName, member avatars, +N badge, and ChevronsUpDown (preserving use of activeWorkspace, initial, displayMembers, activeMembers, getInitials, Avatar/AvatarImage/AvatarFallback). Then replace both branches with either <LearnMoreHoverCard tabId="workspaces" onExpand={onLearnMoreExpand}>{trigger}</LearnMoreHoverCard> when onLearnMoreExpand is truthy or just {trigger} otherwise, ensuring props like tabId/onExpand are kept only on the LearnMoreHoverCard wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@mcpjam-inspector/client/src/components/learn-more/LearnMoreExpandedPanel.tsx`:
- Around line 155-180: The centering math in getInitialStyle assumes PANEL_WIDTH
always equals the rendered panel width, causing wrong finalX/scale when maxWidth
clamps the panel; update getInitialStyle (and the similar logic around lines
203-209) to compute the actual final width used for centering (e.g. finalWidth =
Math.min(PANEL_WIDTH, window.innerWidth - horizontalPadding) or read the
computed layout width) and use finalWidth instead of PANEL_WIDTH when
calculating finalX and scaleX, or alternatively change the animation target to a
centered wrapper/flex container so the transform math targets the wrapper's
center rather than a fixed 900px value.
- Around line 199-249: The panel's root motion.div (key "learn-more-panel", ref
panelRef) should be exposed as a dialog: add role="dialog" and aria-modal="true"
to that element and set aria-labelledby to the id of the title; give the h2 that
renders {entry.title} a stable id (e.g. "learn-more-title" or an id derived from
entry.id) and reference that id from aria-labelledby so assistive tech announces
the panel as a dialog; ensure the close button (onClose) remains
keyboard-focusable and inside the dialog.
---
Nitpick comments:
In
`@mcpjam-inspector/client/src/components/sidebar/sidebar-workspace-selector.tsx`:
- Around line 127-225: The DropdownMenuTrigger/SidebarMenuButton subtree is
duplicated; extract it into a local constant (e.g. const trigger =
(<DropdownMenuTrigger
asChild>...<SidebarMenuButton>...</SidebarMenuButton></DropdownMenuTrigger>))
that includes WorkspaceIconBadge, workspaceName, member avatars, +N badge, and
ChevronsUpDown (preserving use of activeWorkspace, initial, displayMembers,
activeMembers, getInitials, Avatar/AvatarImage/AvatarFallback). Then replace
both branches with either <LearnMoreHoverCard tabId="workspaces"
onExpand={onLearnMoreExpand}>{trigger}</LearnMoreHoverCard> when
onLearnMoreExpand is truthy or just {trigger} otherwise, ensuring props like
tabId/onExpand are kept only on the LearnMoreHoverCard wrapper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 696346c3-6e55-4c23-a449-6bd383d8354d
📒 Files selected for processing (8)
mcpjam-inspector/client/src/components/learn-more/LearnMoreExpandedPanel.tsxmcpjam-inspector/client/src/components/learn-more/LearnMoreHoverCard.tsxmcpjam-inspector/client/src/components/learn-more/__tests__/LearnMoreExpandedPanel.test.tsxmcpjam-inspector/client/src/components/mcp-sidebar.tsxmcpjam-inspector/client/src/components/sidebar/__tests__/sidebar-workspace-selector.test.tsxmcpjam-inspector/client/src/components/sidebar/sidebar-workspace-selector.tsxmcpjam-inspector/client/src/hooks/use-learn-more.tsmcpjam-inspector/client/src/lib/learn-more-content.ts
✅ Files skipped from review due to trivial changes (2)
- mcpjam-inspector/client/src/lib/learn-more-content.ts
- mcpjam-inspector/client/src/hooks/use-learn-more.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- mcpjam-inspector/client/src/components/sidebar/tests/sidebar-workspace-selector.test.tsx
- mcpjam-inspector/client/src/components/learn-more/tests/LearnMoreExpandedPanel.test.tsx
- mcpjam-inspector/client/src/components/learn-more/LearnMoreHoverCard.tsx
Comment on lines
+155
to
+180
| // Compute initial transform from sourceRect to final centered position | ||
| const getInitialStyle = () => { | ||
| if (!sourceRect) { | ||
| // No source (first-visit auto-show) — just scale from center | ||
| return { opacity: 0, scale: 0.95 }; | ||
| } | ||
|
|
||
| const viewW = window.innerWidth; | ||
| const viewH = window.innerHeight; | ||
|
|
||
| // Final position: centered | ||
| const finalX = (viewW - PANEL_WIDTH) / 2; | ||
| const finalY = viewH * 0.1; // 10% from top | ||
|
|
||
| // Offset from center to where the hover card was | ||
| const deltaX = sourceRect.left - finalX; | ||
| const deltaY = sourceRect.top - finalY; | ||
| const scaleX = sourceRect.width / PANEL_WIDTH; | ||
|
|
||
| return { | ||
| opacity: 0.8, | ||
| x: deltaX, | ||
| y: deltaY, | ||
| scale: scaleX, | ||
| transformOrigin: "top left", | ||
| }; |
Contributor
There was a problem hiding this comment.
The centering math breaks once maxWidth clamps the panel.
marginLeft: -(PANEL_WIDTH / 2) and the finalX/scale calculations still assume a 900px panel, but maxWidth can render it much narrower. On smaller viewports the panel shifts off-screen to the left, and the open animation targets the wrong destination. Compute the final width from the constrained viewport size, or center via a wrapper/flex container instead of fixed negative margins.
Also applies to: 203-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/learn-more/LearnMoreExpandedPanel.tsx`
around lines 155 - 180, The centering math in getInitialStyle assumes
PANEL_WIDTH always equals the rendered panel width, causing wrong finalX/scale
when maxWidth clamps the panel; update getInitialStyle (and the similar logic
around lines 203-209) to compute the actual final width used for centering (e.g.
finalWidth = Math.min(PANEL_WIDTH, window.innerWidth - horizontalPadding) or
read the computed layout width) and use finalWidth instead of PANEL_WIDTH when
calculating finalX and scaleX, or alternatively change the animation target to a
centered wrapper/flex container so the transform math targets the wrapper's
center rather than a fixed 900px value.
Comment on lines
+199
to
+249
| <motion.div | ||
| ref={panelRef} | ||
| key="learn-more-panel" | ||
| className="fixed z-50 bg-background rounded-lg border shadow-lg overflow-y-auto overflow-x-hidden" | ||
| style={{ | ||
| top: "10vh", | ||
| left: "50%", | ||
| marginLeft: -(PANEL_WIDTH / 2), | ||
| width: PANEL_WIDTH, | ||
| maxWidth: "calc(100vw - 2rem)", | ||
| maxHeight: "80vh", | ||
| }} | ||
| initial={getInitialStyle()} | ||
| animate={{ | ||
| opacity: 1, | ||
| x: 0, | ||
| y: 0, | ||
| scale: 1, | ||
| transformOrigin: "top left", | ||
| }} | ||
| exit={{ | ||
| opacity: 0, | ||
| scale: 0.97, | ||
| transition: { duration: 0.15, ease: EASING } as any, | ||
| }} | ||
| transition={{ duration: 0.35, ease: EASING }} | ||
| > | ||
| {/* Close button */} | ||
| <button | ||
| onClick={onClose} | ||
| className="absolute top-3 right-3 z-10 rounded-full bg-background/80 backdrop-blur-sm p-1.5 opacity-70 transition-opacity hover:opacity-100 focus:outline-none" | ||
| > | ||
| <XIcon className="h-4 w-4" /> | ||
| <span className="sr-only">Close</span> | ||
| </button> | ||
|
|
||
| {/* Title + docs link */} | ||
| <div className="px-10 pt-8 pb-2 flex items-start justify-between gap-4"> | ||
| <h2 className="text-3xl font-bold leading-tight"> | ||
| {entry.title} | ||
| </h2> | ||
| <a | ||
| href={entry.docsUrl} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="shrink-0 mt-1 flex items-center gap-1 text-sm text-muted-foreground hover:text-foreground transition-colors" | ||
| > | ||
| Docs | ||
| <ExternalLink className="h-3.5 w-3.5" /> | ||
| </a> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Expose the expanded panel as a dialog.
This is modal UI, but assistive tech only sees a generic div. Add role="dialog", aria-modal="true", and connect the heading with aria-labelledby so the panel is announced as an active dialog instead of ordinary page content.
Minimal fix
<motion.div
ref={panelRef}
key="learn-more-panel"
+ role="dialog"
+ aria-modal="true"
+ aria-labelledby="learn-more-title"
className="fixed z-50 bg-background rounded-lg border shadow-lg overflow-y-auto overflow-x-hidden"
style={{
top: "10vh",
@@
- <h2 className="text-3xl font-bold leading-tight">
+ <h2 id="learn-more-title" className="text-3xl font-bold leading-tight">
{entry.title}
</h2>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/learn-more/LearnMoreExpandedPanel.tsx`
around lines 199 - 249, The panel's root motion.div (key "learn-more-panel", ref
panelRef) should be exposed as a dialog: add role="dialog" and aria-modal="true"
to that element and set aria-labelledby to the id of the title; give the h2 that
renders {entry.title} a stable id (e.g. "learn-more-title" or an id derived from
entry.id) and reference that id from aria-labelledby so assistive tech announces
the panel as a dialog; ensure the close button (onClose) remains
keyboard-focusable and inside the dialog.
prathmeshpatel
requested changes
Mar 27, 2026
Collaborator
prathmeshpatel
left a comment
prathmeshpatel
left a comment
There was a problem hiding this comment.
Screen recording demos pls! I want to see what changes look like before approviing
prathmeshpatel
requested changes
Mar 27, 2026
Collaborator
prathmeshpatel
left a comment
prathmeshpatel
left a comment
There was a problem hiding this comment.
I like this direction: UI and most videos look neat and professional! 👏:skin-tone-4:
Feedback (dos are blocking, try and consider are worth addressing, nits are minor and non-blocking)
[do] The videos in the expanded modal should be longer like we aligned
3-5 seconds is perfect for the short video, but the expanded modal I expect ~10s+ like OAuth Debugger
Servers
App Builder
[do] Fix broken docs links
[nit] I should scroll in the expanded modal to see more info
This can be a follow, fine without this for now I guess - TBH I was expecting something more Notion-like like I mentioned last night though.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
workspaces
servers
app builder
skills
oauth debugger