Mount toolbar on slides share + enforce viewer read-only#282
Merged
Conversation
SharedSlidesLayout never mounted SlidesToolbar, so editor-role share links had a canvas with no editing controls. The layout also forwarded `readOnly` to SlidesView only for API parity — the slides editor had no read-only mode, so viewer-role shares could still mutate the deck via clicks, keyboard, thumbnail drag-reorder, and notes typing. Mount SlidesToolbar lazily alongside SlidesView when the share role is editor, matching SharedDocsLayout's pattern. Image upload from the shared toolbar surfaces a toast — the workspace-scoped upload API needs JWT that share-link visitors don't carry. Add an opt-in `readOnly` flag on the three public mount entry points (initializeEditor, mountThumbnailPanel, mountNotesPanel) and thread it through SlidesView. The editor flag short-circuits attachInteractions() so every pointer + document keydown handler is skipped at the bind site; the thumbnail flag clears item.draggable and skips drag/contextmenu handlers; the notes flag marks the textarea readOnly and skips the input handler. SlidesView also skips the empty-deck seed when read-only so a viewer arriving before the owner has saved the first slide never mutates the doc.
The previous commit lazy-loads SlidesToolbar alongside SlidesView in shared-document.tsx for editor-role slides shares. Vite splits the toolbar's contextual subsections (idle / object / text-edit / mobile) into their own dynamic-import chunks, so the build emits ~3 chunks beyond the lazy SlidesView entry — pushing the count from 88 to 91 against the 90 limit. Bump the budget to 93 with a matching note in harness.config.json so the chunk gate stays the canonical record of why each lift happened. Only slides share-link visitors download these chunks; sheets/docs share routes are unaffected.
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 47 minutes and 27 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements read-only mode for viewer-role shared slide links. It gates pointer/keyboard interactions and mutating actions (drag-reorder, context menus, note editing) via optional ChangesRead-Only Shared Slides
Sequence DiagramsequenceDiagram
participant Viewer as Viewer (readOnly=true)
participant Editor as SlidesEditorImpl
participant Thumbs as ThumbnailPanel
participant Notes as NotesPanel
participant Store as SlidesStore
Viewer->>Editor: initialize (readOnly: true)
Editor->>Editor: skip attachInteractions()
Viewer->>Thumbs: mountThumbnailPanel ({readOnly: true})
Thumbs->>Thumbs: set draggable=false, skip drag/context handlers
Viewer->>Notes: mountNotesPanel ({readOnly: true})
Notes->>Notes: textarea.readOnly=true, skip input listener
Viewer->>Thumbs: click thumbnail to navigate
Thumbs->>Editor: update currentSlideIndex
Viewer->>Notes: view notes (read-only)
Note over Thumbs,Notes: No mutations reach Store
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tasks/archive/2026/05/20260523-slides-share-toolbar-lessons.md`:
- Line 36: Update the stale chunk-gate numbers in the markdown line that
currently reads "surface is a small JS-only chunk (chunk gate went 87 → 88 /
90)" to reflect the PR’s final verification values "91 / 93" (with budget 93);
locate the exact sentence in
docs/tasks/archive/2026/05/20260523-slides-share-toolbar-lessons.md and replace
the old "87 → 88 / 90" sequence with "91 / 93" so the recorded verification
matches the PR results.
In `@docs/tasks/archive/2026/05/20260523-slides-share-toolbar-todo.md`:
- Around line 56-57: The sentence "Yorkie write enforcement on the server side
(a viewer that bypasses the UI today would still hit Yorkie ACLs — out of scope
for this PR)." incorrectly claims server-side ACL enforcement; update that line
to remove the security guarantee and clearly state server-side write enforcement
is out of scope for this PR (e.g., reword to indicate only UI-level controls are
addressed and that bypassing the UI may not be blocked by server-side ACLs in
this work). Locate the exact sentence in the document (the line starting "Yorkie
write enforcement on the server side") and replace it with the clarified
phrasing.
🪄 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: f6b38645-edb8-496d-adf8-ba4076521938
📒 Files selected for processing (16)
docs/design/slides/slides.mddocs/tasks/README.mddocs/tasks/archive/2026/05/20260523-slides-share-toolbar-lessons.mddocs/tasks/archive/2026/05/20260523-slides-share-toolbar-todo.mddocs/tasks/archive/README.mdharness.config.jsonpackages/frontend/src/app/shared/shared-document.tsxpackages/frontend/src/app/slides/mobile-slides-view.tsxpackages/frontend/src/app/slides/slides-view.tsxpackages/slides/src/index.tspackages/slides/src/view/editor/editor.tspackages/slides/src/view/editor/notes-panel.tspackages/slides/src/view/editor/thumbnail-panel.tspackages/slides/test/view/editor/editor.test.tspackages/slides/test/view/editor/notes-panel.test.tspackages/slides/test/view/editor/thumbnail-panel.test.ts
Contributor
Verification: verify:selfResult: ✅ PASS in 216.7s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
CodeRabbit flagged two factual drifts in the archived task files: - The lessons file recorded a stale "87 → 88 / 90" chunk gate transition. The actual final state after the lazy SlidesToolbar split is 91 chunks against the new budget of 93, because the toolbar's contextual subsections each emit their own dynamic-import chunk. - The non-goals list claimed "a viewer that bypasses the UI today would still hit Yorkie ACLs," implying server-side enforcement we never verified. Reword to make clear UI-level gating is all this PR adds.
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
SharedSlidesLayoutnever mountedSlidesToolbar, so editor-role share-link visitors saw a canvas with no editing controls. The layout now lazy-mountsSlidesToolbaralongsideSlidesView(gated on!readOnly), mirroringSharedDocsLayout. Image upload from the shared toolbar surfaces an info toast — the workspace-scoped upload API needs JWT that share-link visitors don't carry.SlidesViewacceptedreadOnlyonly for API parity; the underlying editor still bound every pointer + keyboard handler, so viewer-role shares could mutate the deck. Adds an opt-inreadOnlyflag oninitializeEditor,mountThumbnailPanel, andmountNotesPanel, and threads it throughSlidesView—attachInteractions()short-circuits, thumbnail drag/contextmenu handlers are skipped, the notes textarea isreadOnly. The empty-deck seed is also suppressed in read-only mounts.SlidesToolbarsubsections (idle / object / text-edit / mobile); only slides share-link visitors download these chunks.Test plan
pnpm verify:fastgreen.pnpm verify:frontend:chunksgreen (91 / 93).editor.test.ts,thumbnail-panel.test.ts,notes-panel.test.tsexercise thereadOnlygate against the parallel writeable assertions.Known limitations (out of scope)
ensureSlidesRootmay still run adoc.update()migration on viewer mounts of unmigrated pre-v0.5 decks — covered by an in-source comment and owned by the doc-migration workstream.MobileSlidesViewdoes not forwardreadOnlyyet (only mounted from the owner route today); a TODO marks the spot for whenSharedSlidesLayoutgrows a phone-width branch.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation