Add slides presentation mode v1 (local-only fullscreen)#240
Merged
Conversation
Phase 5 of slides.md has reserved a slot for presentation mode, the editor exposes onStartPresentation, and Cmd+Enter is already wired — the handler and the underlying presenter just don't exist yet. This adds the design (single-canvas fullscreen player with keyboard nav, click-to-advance, end screen, overlay fallback) and the 10-task implementation plan. Local-only in v1; presence broadcast and speaker-notes display stay in v2 per the spec. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The presenter is the pure state-machine half of v1 presentation mode: slide-by-slide navigation by ID, end-screen flag, and the public Presenter / PresenterOptions surface. Rendering, input wiring, and fullscreen come in later tasks. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
goToFirst() / goToLast() indexed state.slides without checking for an empty array. next() / prev() are safe via their currentSlideId == null short-circuit, but the jump methods are not. A remote collaborator can empty the deck via Task 5's setDocument once it lands, so the guard ships now while the surface area is tiny. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Fills in paint(): mount a canvas in the container, fit it to the viewport with devicePixelRatio applied to the backing store, and re-render through SlideRenderer on every slide change. The end-screen state paints a black canvas with centered exit text via the raw 2D context — no SlideRenderer involved, since it's not a real slide. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Two small follow-ups from the Task 2 review. Match the thumbnail-panel idiom by feature-detecting ResizeObserver before instantiating, so the presenter degrades gracefully outside browser environments. Hoist the end-screen copy and font-size ratio to module constants so a future i18n pass has a single point to touch. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Capture-phase document keydown listener with a KEY_ACTIONS map for Arrow / Space / PageDown / n / N (next), Arrow / PageUp / Backspace / p / P (prev), Home / End (jump), and Escape (onExit). Every key is swallowed (stopImmediatePropagation + preventDefault) so editor shortcuts can't fire under the presenter — e.g. Cmd+Z won't undo out from under the live deck. Canvas clicks advance, except on the end-screen where they trigger onExit instead. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
requestFullscreen on the container on mount; if denied, fall back to a fixed-position overlay (inset 0, z-index 9999) so iframe- sandboxed or permission-blocked environments still get the same UX minus the native chrome. A document fullscreenchange handler bridges the browser's own Esc back to options.onExit when mounted in fullscreen. The container's cursor hides after 3 s of mousemove inactivity and restores on the next move. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The previous handler called onExit whenever fullscreenchange reported fullscreenElement === null and mountMode was still 'fullscreen', which fires too eagerly in two cases — a stray fullscreenchange before our own requestFullscreen has resolved, and a sibling element on the page exiting fullscreen. Track an enteredFullscreen flag set inside .then, and gate onExit on `document.fullscreenElement === container` so only the presenter's own transitions count. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Concurrent edits on the Yorkie store arrive via setDocument while the presentation runs. Three branches matter: an empty deck triggers onExit so the shell can toast and unmount; an unchanged current-slide id just re-renders so theme and element edits show up; a deleted current slide falls back to the slide at its original index, clamped to the new tail. The end-screen state survives any structural change short of emptying the deck — the presentation is still 'over'. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
dispose() now reverses every install Tasks 2-4 added — canvas, four listeners, ResizeObserver, cursor timeout, fullscreen exit, and the container's prior cssText. The fullscreenchange listener detaches first to avoid the exitFullscreen→handler→onExit→dispose loop. The top-level package index now re-exports startPresenter so the React shell in Task 7 can import it from @wafflebase/slides. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Wraps startPresenter from @wafflebase/slides in a portal-style React component: imperatively creates a div on document.body, mounts the presenter on it, forwards store snapshots into setDocument so remote edits propagate during the presentation, and disposes on unmount. The component renders null itself — its host element is decoupled from React's reconciliation so the presenter can own canvas mount / fullscreen / styles without strict-mode double-mounts splitting them. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Adds the prop to SlidesViewProps and threads it into initializeEditor so the Cmd/Ctrl+Enter shortcuts and the new toolbar Present button (Task 9) route through the same handler in the parent layout. Uses a ref to avoid the editor remounting when the parent re-renders with a fresh callback reference, matching the pattern the React shell landed in Task 7. The "present mode is a separate phase" comment is no longer accurate and is removed. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
SlidesLayout now owns the present-mode state machine: presentingFrom
('current' | 'first' | null) flips when the user clicks the Present
split-button or hits Cmd/Ctrl+Enter, drives a conditional mount of
SlidesPresentationMode, and resets via the presenter's onExit. The
handleStart empty-deck guard covers both the button path and the
shortcut path. The split-button itself sits in the header next to
share and presence chrome, with a dropdown for "Present from
beginning" mirroring Google Slides' UX.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Tick the deferred onStartPresentation item on the keyboard-shortcuts task with a back-reference, and fill the Status section on the presentation-mode task doc with the commit log and the remaining human-driven close-out steps (smoke, lessons, archive). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The end-of-branch review found two real seams. If dispose runs while requestFullscreen is still pending, the browser still honors the request and the page sits in fullscreen with our teardown already done — the .then continuation now exits fullscreen when it sees the disposed flag. If a peer deletes the editor's current slide between the host computing startSlideId and startPresenter running, the presenter would mount on a phantom id; startPresenter now falls back to the first slide. Also: the empty-deck onExit path surfaces a toast, and the design doc drops two stale references (a frontend test file that never landed and the wrong icon library name). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR implements a complete presentation mode for slides. It adds a framework-free presenter in ChangesSlides Presentation Mode v1
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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🧪 Generate unit tests (beta)
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 |
Contributor
Verification: verify:selfResult: ✅ PASS in 178.2s
Verification: verify:integrationResult: ✅ PASS |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
docs/design/slides/slides-presentation-mode.md (1)
167-173: 💤 Low valueAdd language identifier to the code fence.
The code block starting at line 167 is missing a language identifier. While it's pseudo-code illustrating the fullscreen fallback pattern, adding an identifier (e.g.,
typescriptorjavascript) improves syntax highlighting and tooling support.📝 Suggested fix
-``` +```typescript try { await container.requestFullscreen(); } catch {As per coding guidelines, the static analysis tool
markdownlint-cli2flagged this asMD040, fenced-code-language.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/slides/slides-presentation-mode.md` around lines 167 - 173, The fenced code block demonstrating the fullscreen fallback is missing a language identifier which triggers markdownlint MD040; update the code fence to include a language (e.g., add "```typescript" or "```javascript") for the block containing the try { await container.requestFullscreen(); } catch { applyOverlayStyles(container); } so syntax highlighting and tooling will recognize it.packages/frontend/src/app/slides/slides-present-button.tsx (1)
22-26: 💤 Low valueConsider future-proofing platform detection.
navigator.platformis deprecated in favor ofnavigator.userAgentData.platform. The current approach works and is widely compatible, but you may want to add a fallback chain for future-proofing:♻️ Proposed enhancement
const modKey = typeof navigator !== "undefined" && - navigator.platform.toLowerCase().includes("mac") + ((navigator.userAgentData?.platform?.toLowerCase().includes("mac")) ?? + navigator.platform.toLowerCase().includes("mac")) ? "⌘" : "Ctrl";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/frontend/src/app/slides/slides-present-button.tsx` around lines 22 - 26, Update the platform detection used to derive modKey to use navigator.userAgentData.platform when available, falling back to navigator.userAgent and then navigator.platform (and finally a safe default); modify the logic around the modKey constant (variable name: modKey) to first check typeof navigator !== "undefined" && navigator.userAgentData && navigator.userAgentData.platform, then test that value for "mac" (case-insensitive), else fall back to navigator.userAgent (searching for "Mac" / "Macintosh") and then navigator.platform as currently implemented, so mac detection remains correct and future-proof.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/frontend/src/app/slides/slides-detail.tsx`:
- Around line 245-260: The code passes a non-null assertion to
SlidesPresentationMode using resolveStartSlideId which can return undefined
during a race (e.g., remote delete); fix by guarding the call: compute const
startSlideId = resolveStartSlideId(store, presentingFrom, editor) and only
render <SlidesPresentationMode ... startSlideId={startSlideId} /> when
startSlideId is a string (or alternatively make resolveStartSlideId throw when
the deck is empty and catch/avoid rendering); reference resolveStartSlideId,
SlidesPresentationMode, presentingFrom, store, editor, and setPresentingFrom
when applying the guard.
In `@packages/slides/src/view/present/presenter.ts`:
- Around line 67-77: The computed resolvedStartId can crash when
options.doc.slides is empty; update the logic that computes resolvedStartId to
first check that options.doc.slides.length > 0 before accessing slides[0].id
(use options.startSlideId when it exists and matches a slide, otherwise if
slides is non-empty use slides[0].id, and if slides is empty return a safe
fallback such as undefined or throw a clear error). Change the code around
resolvedStartId to reference options.doc.slides.length and options.startSlideId
explicitly so you never index slides[0] when the array may be empty.
- Around line 366-379: The optional chaining on container.requestFullscreen
followed by .then() can call .then on undefined; fix by guarding the call: check
if container.requestFullscreen exists before invoking it and attach .then/.catch
only when it does; if it doesn't exist, run the fallback that sets mountMode =
'overlay' and calls applyOverlayStyles() (the same behavior currently in the
catch branch). Ensure the existing logic that checks disposed, sets
enteredFullscreen = true, and calls document.exitFullscreen?.().catch(...)
remains inside the success handler for requestFullscreen() so those symbols
(container.requestFullscreen, enteredFullscreen, disposed,
document.exitFullscreen, mountMode, applyOverlayStyles) are used correctly.
---
Nitpick comments:
In `@docs/design/slides/slides-presentation-mode.md`:
- Around line 167-173: The fenced code block demonstrating the fullscreen
fallback is missing a language identifier which triggers markdownlint MD040;
update the code fence to include a language (e.g., add "```typescript" or
"```javascript") for the block containing the try { await
container.requestFullscreen(); } catch { applyOverlayStyles(container); } so
syntax highlighting and tooling will recognize it.
In `@packages/frontend/src/app/slides/slides-present-button.tsx`:
- Around line 22-26: Update the platform detection used to derive modKey to use
navigator.userAgentData.platform when available, falling back to
navigator.userAgent and then navigator.platform (and finally a safe default);
modify the logic around the modKey constant (variable name: modKey) to first
check typeof navigator !== "undefined" && navigator.userAgentData &&
navigator.userAgentData.platform, then test that value for "mac"
(case-insensitive), else fall back to navigator.userAgent (searching for "Mac" /
"Macintosh") and then navigator.platform as currently implemented, so mac
detection remains correct and future-proof.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67278fe2-d120-48e9-a1a9-698746ea1d9b
📒 Files selected for processing (13)
docs/design/README.mddocs/design/slides/slides-presentation-mode.mddocs/design/slides/slides.mddocs/tasks/active/20260514-slides-keyboard-shortcuts-todo.mddocs/tasks/active/20260514-slides-presentation-mode-todo.mdpackages/frontend/src/app/slides/slides-detail.tsxpackages/frontend/src/app/slides/slides-present-button.tsxpackages/frontend/src/app/slides/slides-presentation-mode.tsxpackages/frontend/src/app/slides/slides-view.tsxpackages/slides/src/index.tspackages/slides/src/view/present/index.tspackages/slides/src/view/present/presenter.test.tspackages/slides/src/view/present/presenter.ts
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Three real seams plus a doc-fence nit. The optional-chain bug on container.requestFullscreen meant a missing Fullscreen API would crash with "Cannot read .then of undefined" rather than fall through to the overlay path — explicit feature-detect now bridges that. startPresenter now throws fail-fast when handed an empty deck, since the host empty-deck guard is the only thing standing between the public API and an array-index crash, and a future caller may bypass the React shell. The "!" non-null assertion in the layout's conditional mount is replaced with an explicit guard so a remote empty-deck race between setPresentingFrom and the next render no longer fabricates an undefined startSlideId. Design-doc fenced block gets a typescript language identifier. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
4 tasks
hackerwins
added a commit
that referenced
this pull request
May 18, 2026
These seven slides + PPTX tasks shipped (PRs #238, #240, #241, #255, #259, #260, #261) but their checklist boxes were never ticked during execution, so the archive script left them in active/. Sweep the checkboxes to reflect what actually landed, add lessons files for the two that were missing (keyboard-shortcuts, presentation-mode), and convert the keyboard-shortcuts onLinkRequest popover bullet from a checkbox to a "Deferred (follow-up)" note so the gate stops blocking on work that is intentionally out of scope. Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
1 task
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
1 participant
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.
Summary
docs/design/slides/slides.mdhas reserved a slot for. Editor already exposes
onStartPresentationand theCmd/Ctrl+Entershortcut keys are wired — this PR fills in the handler andthe underlying presenter.
startPresenter(options)module inpackages/slides/src/view/present/(~450 LOC, 52 unit tests) coveringfullscreen, overlay fallback, keyboard nav, click-to-advance, end-screen,
cursor auto-hide, identity-gated
fullscreenchange, ID-trackedremote-change handling, and clean dispose.
SlidesPresentationMode(portal-style, rendersnull)plus a
<PresentButton>split-button inSiteHeadernext to share /presence chrome. Cmd/Ctrl+Enter routes through the same handler as the
button via a ref-wrapped
onStartPresentationprop, so a fresh parentcallback identity doesn't tear down the editor.
auto-snapped to the presenter's slide. Speaker-notes display and dual-
screen presenter view remain v2 per the design doc.
Design
docs/design/slides/slides-presentation-mode.md— full v1 spec.docs/design/slides/slides.mdPhase 5 section now cross-references thenew doc and is downscoped to local-only.
Test plan
pnpm verify:fastgreen on every commit (47 test files, 764 tests).packages/slides/src/view/present/presenter.test.tscover navigation,canvas mount, end-screen text, keyboard / click, cursor auto-hide,
fullscreen, overlay fallback, identity-gated
fullscreenchange,dispose-during-pending-fullscreen race, stale-
startSlideIdfallback,remote-change handling, and dispose cleanup.
pnpm dev):letterboxed, current slide visible.
Click on canvas advances.
Esc exits to editor.
Cmd+Enterfrom a non-text-focused state starts present mode.Cmd+Enterwhile editing a text box does NOT enter (gated).fullscreenchange→onExit).Delete current slide in tab B → tab A jumps to the slide at the
same index (or last). Delete all slides → tab A exits with toast
'Presentation ended (deck is empty)'.
fullscreen race): page should not be stuck in fullscreen.
Notes
@wafflebase/slides:startPresenter,Presenter,PresenterOptions.onStartPresentationwiring TODO on20260514-slides-keyboard-shortcuts-todo.md.docs/tasks/active/20260514-slides-presentation-mode-todo.mdtask-by-task with per-task spec + code-quality reviews plus a final
cross-cutting review; Status section at the bottom of that doc lists all
16 commits.
Summary by CodeRabbit
Release Notes
New Features
Documentation