Conversation
- Introduced a new CSS theme for slides presentation in `theme-markview.css`. - Implemented `MarkdownViewer` tests to verify slides mode functionality, including page flipping and keyboard navigation. - Created `SlidesToggle` component to manage the visibility of slides, with corresponding tests to ensure correct behavior and accessibility attributes.
There was a problem hiding this comment.
Code Review
This pull request introduces a new built-in Slides presentation mode for Markdown files, featuring slide navigation, full-screen capabilities, and auto-hiding overlay controls. It also adds Marp slide templates, Makefile targets for slide preview/export, and a safe pnpm installation script to handle architecture switches. The review feedback focuses on refining the React implementation: typing the overlay timer reference to prevent TypeScript compilation issues, throttling mouse movement events to optimize performance, preventing slide progression during text selection, and preserving keyboard accessibility for interactive elements on slides.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const slideShellRef = useRef<HTMLDivElement>(null); | ||
| const slidesOverlayTimerRef = useRef<number | null>(null); |
There was a problem hiding this comment.
To avoid potential TypeScript compilation errors in environments where Node.js types are loaded globally (where setTimeout returns a Timeout object instead of a number), it is highly recommended to type the slidesOverlayTimerRef using ReturnType<typeof setTimeout>. Additionally, we can introduce a lastOverlayResetRef to throttle overlay visibility timer resets during continuous mouse movement.
| const slideShellRef = useRef<HTMLDivElement>(null); | |
| const slidesOverlayTimerRef = useRef<number | null>(null); | |
| const slideShellRef = useRef<HTMLDivElement>(null); | |
| const slidesOverlayTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); | |
| const lastOverlayResetRef = useRef<number>(0); |
| const handleSlidesOverlayActivity = useCallback(() => { | ||
| revealSlidesOverlay(); | ||
| }, [revealSlidesOverlay]); |
There was a problem hiding this comment.
The onMouseMove event fires extremely frequently (dozens of times per second). Resetting the overlay timer on every single mouse movement by clearing and setting timeouts can cause performance stuttering, especially on lower-end devices. Throttling the timer resets to at most once every 200ms significantly reduces CPU overhead while maintaining the same user experience.
const handleSlidesOverlayActivity = useCallback(() => {
const now = Date.now();
if (now - lastOverlayResetRef.current > 200) {
lastOverlayResetRef.current = now;
revealSlidesOverlay();
}
}, [revealSlidesOverlay]);
| const handleSlidePageClick = useCallback( | ||
| (event: React.MouseEvent<HTMLElement>) => { | ||
| const target = event.target as HTMLElement | null; | ||
| if (!target) return; | ||
| if (target.closest("a,button,input,textarea,select,label,summary,[role='button']")) { | ||
| return; | ||
| } | ||
| goNextSlide(); | ||
| }, | ||
| [goNextSlide], | ||
| ); |
There was a problem hiding this comment.
When a user attempts to select and copy text from a slide, clicking and dragging or double-clicking will trigger the slide click handler and unexpectedly advance to the next slide. Checking if there is active text selection via window.getSelection() before advancing prevents this frustrating user experience.
const handleSlidePageClick = useCallback(
(event: React.MouseEvent<HTMLElement>) => {
const target = event.target as HTMLElement | null;
if (!target) return;
if (target.closest("a,button,input,textarea,select,label,summary,[role='button']")) {
return;
}
// Avoid advancing slides when user is selecting text
if (window.getSelection()?.toString()) {
return;
}
goNextSlide();
},
[goNextSlide],
);
| const onKeyDown = (event: KeyboardEvent) => { | ||
| const target = event.target as HTMLElement | null; | ||
| if ( | ||
| target && | ||
| (target.tagName === "INPUT" || | ||
| target.tagName === "TEXTAREA" || | ||
| target.tagName === "SELECT" || | ||
| target.isContentEditable) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| if (isSlidesFullscreen) { | ||
| revealSlidesOverlay(); | ||
| } | ||
|
|
||
| if (["ArrowRight", "PageDown", " ", "Enter"].includes(event.key)) { | ||
| event.preventDefault(); | ||
| goNextSlide(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When keyboard focus is on an interactive element (such as a button, link, or summary) within the slide, pressing Space or Enter should activate that element rather than navigating to the next slide. Intercepting these keys globally breaks standard keyboard accessibility (a11y). We should bypass slide navigation for Space and Enter when focusing interactive elements.
const onKeyDown = (event: KeyboardEvent) => {
const target = event.target as HTMLElement | null;
if (
target &&
(target.tagName === "INPUT" ||
target.tagName === "TEXTAREA" ||
target.tagName === "SELECT" ||
target.isContentEditable)
) {
return;
}
// Avoid intercepting Space/Enter when focusing interactive elements
const isInteractive = target && (
target.tagName === "BUTTON" ||
target.tagName === "A" ||
target.tagName === "SUMMARY" ||
target.getAttribute("role") === "button"
);
if (isInteractive && (event.key === " " || event.key === "Enter")) {
return;
}
if (isSlidesFullscreen) {
revealSlidesOverlay();
}
if (["ArrowRight", "PageDown", " ", "Enter"].includes(event.key)) {
event.preventDefault();
goNextSlide();
return;
}
No description provided.