✨(frontend) Improve presenter core behavior#2482
Draft
PanchoutNathan wants to merge 12 commits into
Draft
Conversation
Contributor
|
Size Change: +917 B (+0.02%) Total Size: 4.37 MB 📦 View Changed
|
WalkthroughThis PR restructures the document presenter feature: a global Zustand store ( Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx (1)
42-65: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThread the store’s starting slide index into this overlay.
The new presenter contract is supposed to carry a starting slide index, but this component now only accepts
{ doc, onClose }and prepends a title slide at index0. Since Line 66 still hard-codesuseState(0), every open lands on the title slide and callers can no longer start the deck at the requested content slide.🤖 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 `@src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx` around lines 42 - 65, The PresenterOverlay component is ignoring the starting slide index from the presenter contract, so it always opens at the title slide. Update PresenterOverlayProps and the PresenterOverlay signature to accept and use the incoming start index, then thread that value into the local slide state instead of hard-coding the initial index. Make sure the slides array built from title and contentSlides still aligns with the passed index when opening.
🤖 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
`@src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterRoot.tsx`:
- Around line 51-86: The PresenterRoot component only stops rendering when
active becomes false, but it never resets the presenter Zustand state, so isOpen
can remain true across mobile resize or unmount/remount. Update PresenterRoot to
close the presenter in a useEffect when active turns false, using the existing
close action from usePresenterStore alongside handleClose, so the global
presenter state is cleared whenever the root deactivates.
In
`@src/frontend/apps/impress/src/features/docs/doc-presenter/stores/usePresenterStore.tsx`:
- Around line 14-15: The open handler in usePresenterStore still allows
non-integer or non-finite values to be stored in initialIndex, which can break
slide selection. Update the open function to sanitize index before calling set
by coercing it to a finite integer and falling back to 0 for NaN, Infinity, and
fractional or invalid inputs. Keep the fix localized to the open method and
ensure initialIndex always represents a valid 0-based slide position.
In `@src/frontend/apps/impress/src/features/docs/doc-presenter/types.ts`:
- Around line 15-18: `PresenterContentSlide.blocks` is still typed as
`unknown[]`, which forces unsafe casts in the presenter pipeline. Update the
`PresenterContentSlide` type in `types.ts` to use `PresenterBlock[]` instead,
and ensure the shared slide contract matches the shape already produced by
`useSlides` and consumed by `PresenterOverlay`/`getSlideTitle` so downstream
code can rely on type safety without casts.
---
Outside diff comments:
In
`@src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx`:
- Around line 42-65: The PresenterOverlay component is ignoring the starting
slide index from the presenter contract, so it always opens at the title slide.
Update PresenterOverlayProps and the PresenterOverlay signature to accept and
use the incoming start index, then thread that value into the local slide state
instead of hard-coding the initial index. Make sure the slides array built from
title and contentSlides still aligns with the passed index when opening.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 757c0d8c-a494-411d-949d-ec8e78e7f2d4
📒 Files selected for processing (15)
src/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.tssrc/frontend/apps/e2e/__tests__/app-impress/utils-common.tssrc/frontend/apps/impress/src/features/docs/doc-header/components/DocToolBox.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/useSlides.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterRoot.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterSlide.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterSlideContent.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterTitleSlide.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/useSlides.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/index.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/stores/index.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/stores/usePresenterStore.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/types.tssrc/frontend/apps/impress/src/pages/docs/[id]/index.tsx
PanchoutNathan
changed the title
AntoLC
reviewed
Jul 7, 2026
createDoc returned before the ProseMirror editor was visible, so tests that immediately typed or opened the presenter raced the load. Wait for the editor container before handing control back.
Move presenter ownership out of DocToolBox into a top-level PresenterRoot driven by a zustand store, lazy-load the overlay and paint a boot cover while the editor boots. Groundwork for the deep-link and present-from-here features (#2466).
Pull the BlockNote rendering out of PresenterSlide into PresenterSlideContent so it can be reused by the title slide and the PDF export. Add a PresenterSlideData union (title | content) and a shared PresenterBlock type.
Prepend a slide with the document title (emoji stripped) so every presentation opens on a cover, announced to screen readers like any other slide (#2466).
Dividers with children are kept as structural parents so their slide keeps its indentation; the divider's own rule is hidden. Empty paragraphs around a divider are dropped so habitual spacing no longer offsets slides. Closes #2398
A selected image (or any selected node) leaked its editing outline into the read-only slide. Suppress the selected-node outline so nothing looks focused unless the user tabs or clicks. Closes #2393
PanchoutNathan
force-pushed
the
feature/present-mode-core
branch
from
577544d to
86a56f4
Compare
AntoLC
reviewed
Jul 8, 2026
AntoLC
left a comment
AntoLC
left a comment
Collaborator
There was a problem hiding this comment.
Except the e2e commit, LGTM !
Can you squash your fixup commit ?
Comment on lines
+158
to
+161
| await page.waitForURL(`**/docs/${docId}**`, { | ||
| timeout: 10000, | ||
| }); | ||
|
|
Collaborator
There was a problem hiding this comment.
We have this part quite stable, I am reluctant to add any change here, and if so, it should be in a dedicated PR.
Ovgodd
requested changes
Jul 8, 2026
Ovgodd
left a comment
Ovgodd
left a comment
Collaborator
There was a problem hiding this comment.
Just a suggestion for long slides accessibility
| if (pm) { | ||
| pm.removeAttribute('role'); | ||
| pm.removeAttribute('contenteditable'); | ||
| pm.setAttribute('tabindex', '-1'); |
Collaborator
There was a problem hiding this comment.
Consider setting tabindex="0" on the slide content. With tabindex="-1", keyboard users cannot reach long slides via Tab and cannot scroll their content.
Suggested change
| pm.setAttribute('tabindex', '-1'); | |
| pm.setAttribute('tabindex', '0'); |
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.
Purpose
Refs #2466
Closes #2398
Closes #2393