slides: add read-only mobile view (<768px)#255
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds a complete mobile slides viewer experience for viewports under 768px, featuring pointer-swipe gesture navigation, canvas-based rendering, and responsive layout branching, alongside comprehensive design documentation and task planning spanning six implementation steps. ChangesMobile Slides View Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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📝 Generate docstrings
🧪 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 |
![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
🤖 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 `@docs/design/slides/slides-mobile-view.md`:
- Around line 94-110: Update the fenced code block that contains the
JSX/HTML-like DOM snippet to include a language identifier (use "html") so
markdown linting and syntax highlighting pass; locate the block showing the <div
class="mobile-slides"> ... <canvas /> ... </div> (references: mobile-slides,
canvasHostRef, canvas) and change the opening triple backticks from ``` to
```html.
In `@packages/frontend/src/app/slides/slides-detail.tsx`:
- Around line 66-70: The early return in SlidesLayout bypasses the shared
document fetching and not-found/permission redirect logic currently used inside
DesktopSlidesLayout; move the document query and any redirect/validation (the
fetchDocument or equivalent logic that produces the resolved title and not-found
behavior) up into SlidesLayout before checking useIsMobile(), then branch to
MobileSlidesView and DesktopSlidesLayout passing the resolved title (e.g., prop
name title) into MobileSlidesView (and keep passing it into DesktopSlidesLayout
if needed) so both mobile and desktop share the same existence/permission
guardrails and API title source.
In `@packages/frontend/src/hooks/use-pointer-swipe.ts`:
- Around line 52-67: The onDown handler currently resets
startX/startY/startTime/pointerId even if a gesture is active; modify onDown
(function onDown) to ignore additional pointerdown events by returning early
when a tracked pointer is already active (e.g., if pointerId != null or phase
=== "pending" or "active"), so only the first down initializes startX, startY,
startTime and pointerId and only that pointer attempts setPointerCapture; ensure
you clear pointerId when the pointer ends/cancels elsewhere so new downs are
accepted.
🪄 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: 16d0ecf6-01eb-4b32-97db-e52a35a54a11
📒 Files selected for processing (10)
docs/design/README.mddocs/design/slides/slides-mobile-view.mddocs/tasks/README.mddocs/tasks/active/20260517-slides-mobile-view-lessons.mddocs/tasks/active/20260517-slides-mobile-view-todo.mddocs/tasks/archive/README.mdpackages/frontend/src/app/slides/mobile-slides-view.tsxpackages/frontend/src/app/slides/slides-detail.tsxpackages/frontend/src/hooks/use-pointer-swipe.tspackages/frontend/tests/hooks/use-pointer-swipe.test.ts
hackerwins
added a commit
that referenced
this pull request
May 17, 2026
A second pointerdown landing mid-swipe (common iOS/Android two-finger scenario) was silently overwriting startX/startY/ pointerId, so the first finger's gesture either misfired or produced no swipe at all. The fix is a one-line active-pointer guard in onDown; both regression cases (mid-gesture second finger, fresh gesture after pointercancel) are covered by new unit tests. Also tags the markdown DOM-tree code block in docs/design/slides/slides-mobile-view.md with `html` so linters syntax-highlight it correctly. Both issues raised by CodeRabbit on PR #255. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
hackerwins
added a commit
that referenced
this pull request
May 17, 2026
The useIsMobile branch in SlidesLayout previously skipped past DesktopSlidesLayout's document fetchDocument + not-found redirect, so navigating to /p/<bad-id> on mobile would attach to an empty Yorkie doc instead of redirecting away — a real access-control gap, not just a UX miss, since Yorkie does not consult the backend's document existence check. Moves both the document and workspaces queries plus the toast + redirect up to SlidesLayout (parent), drops the now-duplicated redirect from DesktopSlidesLayout (react-query's cache means its inner useQuery is free), and passes the resolved title down to MobileSlidesView so the mobile header matches the document API rather than relying on the Yorkie meta title. Raised by CodeRabbit on PR #255. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Captures the design for a read-only mobile experience: SlidesView branches to a dedicated MobileSlidesView component below 768px, the editor module is not mounted (read-only by construction), and SlideRenderer is reused directly the way presenter.ts does. Adds the matching todo + lessons scaffold so the implementation can land as small commits on this branch. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Encapsulates horizontal-swipe classification so the upcoming mobile slides view can hand off gesture detection. Locks horizontal intent once |dx| > 10px (to suppress iOS swipe-back where allowed), falls back to no-op below a 50px / 600ms threshold, and is split into a pure attach helper plus a thin React wrapper because the frontend test runner is node:test and cannot drive React components. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Read-only mobile shell for slide decks: thin header (back / title / Present), an empty canvas host that's wired to usePointerSwipe for left/right slide navigation, footer indicator with explicit arrow buttons as the screen-reader fallback, and an inline SlidesPresentationMode mount so the Present button launches the same fullscreen presenter the desktop view uses. Canvas painting lands in the next commit; this commit keeps the React shell reviewable in isolation. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Wires a SlideRenderer to MobileSlidesView's canvas. A RAF-coalesced ResizeObserver rebuilds the renderer on host-size changes (cheap constructor, same pattern as presenter.ts), and the store's onChange callback triggers a fresh read+paint when peers edit the deck. Re-uses the same computeFitSize math as slides-view.tsx / presenter.ts — intentional ~10-line duplicate per the design doc since the slides package must stay frontend-agnostic. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Adds a useIsMobile branch at the top of SlidesLayout. When the viewport is narrower than 768px the layout swaps the entire desktop chrome (SidebarProvider, AppSidebar, SiteHeader, SlidesToolbar, SlidesView, ThemePanel) for MobileSlidesView, which is read-only by construction since the editor module is never mounted. Crossing the breakpoint at runtime unmounts one tree and mounts the other; the surrounding DocumentProvider keeps the Yorkie attachment alive across the swap. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Capture the three plan deviations made during implementation so the next mobile-related task starts with the right priors: node:test (not Vitest) and the .tsx stub policy, the need to branch in SlidesLayout (not SlidesView) so the desktop chrome is fully replaced, and the deferral of the visual baseline scenario for the mobile chrome. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The previous docstring read "canvas painting lands in the next commit", which was accurate when the scaffold landed but is misleading now that the renderer is wired up. Rewrites the JSDoc to point at the SlideRenderer reuse instead. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
A second pointerdown landing mid-swipe (common iOS/Android two-finger scenario) was silently overwriting startX/startY/ pointerId, so the first finger's gesture either misfired or produced no swipe at all. The fix is a one-line active-pointer guard in onDown; both regression cases (mid-gesture second finger, fresh gesture after pointercancel) are covered by new unit tests. Also tags the markdown DOM-tree code block in docs/design/slides/slides-mobile-view.md with `html` so linters syntax-highlight it correctly. Both issues raised by CodeRabbit on PR #255. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The useIsMobile branch in SlidesLayout previously skipped past DesktopSlidesLayout's document fetchDocument + not-found redirect, so navigating to /p/<bad-id> on mobile would attach to an empty Yorkie doc instead of redirecting away — a real access-control gap, not just a UX miss, since Yorkie does not consult the backend's document existence check. Moves both the document and workspaces queries plus the toast + redirect up to SlidesLayout (parent), drops the now-duplicated redirect from DesktopSlidesLayout (react-query's cache means its inner useQuery is free), and passes the resolved title down to MobileSlidesView so the mobile header matches the document API rather than relying on the Yorkie meta title. Raised by CodeRabbit on PR #255. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Picks up two entries that landed on main while this branch was in review (docs-comments-followup and slides-textbox-click-scale) and bumps the archive count from 181 to 184. Auto-generated by pnpm tasks:index. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
hackerwins
force-pushed
the
slides-mobile-view
branch
from
May 17, 2026 12:56
e6fae82 to
f68a9f1
Compare
Contributor
Verification: verify:selfResult: ✅ PASS in 209.6s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/design/slides/slides-mobile-view.md (1)
67-78: ⚡ Quick winDesign document inconsistencies with implementation.
The review stack context indicates the implementation diverged from this design specification in two areas:
Component location (lines 67, 224): The document states branching occurs in
SlidesView(slides-view.tsx), but the actual implementation places this logic inSlidesLayout(slides-detail.tsx). Per the review stack context: "UpdatesSlidesLayoutto own document fetch and redirect-on-error guard for both mobile and desktop."Data flow approach (lines 119-131): The document specifies using
doc.subscribedirectly while avoidingYorkieSlidesStore, but the implementation actually "instantiates and syncsYorkieSlidesStore" according to the stack context for the mobile slides view component layer.📝 Suggested alignment
Update the design document to reflect the as-built implementation:
Section "Detection and branching" (line 67):
-`SlidesView` (`packages/frontend/src/app/slides/slides-view.tsx`) +`SlidesLayout` (`packages/frontend/src/app/slides/slides-detail.tsx`) branches at the top of its render based on the existingSection "Yorkie data flow" (lines 119-131):
-- The deck's slides, theme, and meta are read from the Yorkie root - into React state. A `doc.subscribe((e) => ...)` listener triggers - a re-snapshot on `remote-change` events; this mirrors the pattern - used by `yorkie-slides-store.ts` and avoids a heavier - `YorkieSlidesStore` instance, which the mobile view does not - need. No writes are issued. +- A `YorkieSlidesStore` instance is instantiated and synchronized with + the Yorkie document. The store's snapshot is subscribed to track + remote changes, providing slides, theme, and metadata to the React + component. No mutation operations are called on the store.File change summary table (line 224):
-| `packages/frontend/src/app/slides/slides-view.tsx` | Add `useIsMobile()` branch at the top of render; delegate to `MobileSlidesView` when true. | +| `packages/frontend/src/app/slides/slides-detail.tsx` | Add `useIsMobile()` branch in `SlidesLayout`; mount `MobileSlidesView` for mobile, `DesktopSlidesLayout` otherwise. |Also applies to: 119-131, 224-224
🤖 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-mobile-view.md` around lines 67 - 78, The design doc is out-of-sync with the implementation: the branching for mobile/desktop lives in SlidesLayout (slides-detail.tsx) not SlidesView (slides-view.tsx), and the data flow uses YorkieSlidesStore (instantiated/synced) instead of directly calling doc.subscribe; update the "Detection and branching" section to state that SlidesLayout performs the useIsMobile-based branch and mention useIsMobile, and update the "Yorkie data flow" section to describe instantiation and sync behavior of YorkieSlidesStore (and where it is used) rather than doc.subscribe; also update the file change summary to reflect SlidesLayout owning document fetch and redirect-on-error guard and that YorkieSlidesStore is used for mobile slides.
🤖 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 77-113: The document view is mounting before the document query
settles, risking Yorkie attachment for invalid IDs; update the component to gate
rendering by using the document query's isLoading and isError state: add
isLoading from the useQuery that fetches document (alongside isDocumentError),
and if isLoading return null (or a placeholder) so neither MobileSlidesView nor
DesktopSlidesLayout mounts; additionally, if isDocumentError return null (so
nothing mounts) while the useEffect handles toast+navigate; reference the
document fetch useQuery, isDocumentError/isLoading, documentData, fallbackSlug,
isMobile, MobileSlidesView and DesktopSlidesLayout when making the change.
---
Nitpick comments:
In `@docs/design/slides/slides-mobile-view.md`:
- Around line 67-78: The design doc is out-of-sync with the implementation: the
branching for mobile/desktop lives in SlidesLayout (slides-detail.tsx) not
SlidesView (slides-view.tsx), and the data flow uses YorkieSlidesStore
(instantiated/synced) instead of directly calling doc.subscribe; update the
"Detection and branching" section to state that SlidesLayout performs the
useIsMobile-based branch and mention useIsMobile, and update the "Yorkie data
flow" section to describe instantiation and sync behavior of YorkieSlidesStore
(and where it is used) rather than doc.subscribe; also update the file change
summary to reflect SlidesLayout owning document fetch and redirect-on-error
guard and that YorkieSlidesStore is used for mobile slides.
🪄 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: 9b41aced-cd8f-4cdb-91e0-ce3ccf591124
📒 Files selected for processing (9)
docs/design/README.mddocs/design/slides/slides-mobile-view.mddocs/tasks/README.mddocs/tasks/active/20260517-slides-mobile-view-lessons.mddocs/tasks/active/20260517-slides-mobile-view-todo.mdpackages/frontend/src/app/slides/mobile-slides-view.tsxpackages/frontend/src/app/slides/slides-detail.tsxpackages/frontend/src/hooks/use-pointer-swipe.tspackages/frontend/tests/hooks/use-pointer-swipe.test.ts
✅ Files skipped from review due to trivial changes (3)
- docs/design/README.md
- docs/tasks/active/20260517-slides-mobile-view-lessons.md
- docs/tasks/active/20260517-slides-mobile-view-todo.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/frontend/src/hooks/use-pointer-swipe.ts
The hoist in 8c028b8 moved the not-found redirect into SlidesLayout, but the useEffect that fires the toast + navigate runs only after the first paint. In that window both branches would mount and trigger a Yorkie attach for the requested id before the backend access check had landed — a peer with the deck already open would briefly leak contents through the Yorkie subscription. Holds both branches behind a Loader while the document query is in-flight and returns null once the error is known so the redirect's race window closes. Raised by CodeRabbit on PR #255. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
hackerwins
added a commit
that referenced
this pull request
May 17, 2026
Below 768px the desktop chrome (sidebar + site header + toolbar) ate the viewport and left the slide canvas unreadable. This PR adds a dedicated MobileSlidesView — thin header (back / title / Present), full-width canvas, footer indicator with arrow buttons and swipe navigation — that SlidesLayout mounts in place of the desktop tree when useIsMobile() reports a narrow viewport. Read-only is enforced by construction; the editor module is never mounted on mobile, so no mutation pathway exists. SlideRenderer is reused directly the same way the presenter does, so the canvas pipeline stays in one place. usePointerSwipe wraps a pure-DOM gesture core (unit-tested against a fake element — the frontend test runner is node:test and stubs .tsx files, so the React wrapper is covered by manual smoke). Code review surfaced three correctness items handled on the same branch: the document existence/permission check is hoisted up to SlidesLayout so the mobile branch is no longer an access-control bypass; the mount is gated on the document query settling so a peer with the deck already open cannot briefly leak contents through the Yorkie subscription; and a multi-finger guard in usePointerSwipe prevents a second pointerdown from overwriting an in-flight gesture's start coordinates. Design doc: docs/design/slides/slides-mobile-view.md Lessons: docs/tasks/active/20260517-slides-mobile-view-lessons.md
5 tasks
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]>
14 tasks
4 tasks
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
MobileSlidesViewshell — thin header (back / title / Present), full-width slide canvas, footer indicator, and left/right swipe nav — mounted bySlidesLayoutwhenuseIsMobile()is true.SlideRendereris reused directly the same wayview/present/presenter.tsdoes.usePointerSwipe/attachPointerSwipepair; the pure-DOM core is unit-tested against a fake element (the frontend test runner isnode:testand stubs.tsx, so the React wrapper itself is covered only by manual smoke).docs/design/slides/slides-mobile-view.md. Implementation lessons (plan deviations) captured indocs/tasks/active/20260517-slides-mobile-view-lessons.md.Test plan
pnpm verify:fastgreen (792 tests)pnpm verify:selfgreen (sheets/docs/slides/frontend/backend/cli builds + entropy)DocumentProviderKnown limitations
MobileSlidesViewis tightly coupled touseDocument. Detail in the lessons file.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests