✨(frontend) add the presenter mode feature#2321
Conversation
PanchoutNathan
changed the title
|
Size Change: +5.62 kB (+0.13%) Total Size: 4.32 MB 📦 View Changed
|
WalkthroughThis PR introduces a complete presenter mode feature for document presentations. The implementation includes four foundational hooks (fullscreen control, slide scaling, keyboard shortcuts, slide partitioning), three UI components (PresenterFloatingBar, PresenterSlide, PresenterOverlay), integration into DocToolBox with a "Present" dropdown option, presentation tuning constants, unit and E2E tests, and a SideModal TypeScript type refinement. CHANGELOG updated. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 4
🤖 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 `@CHANGELOG.md`:
- Around line 19-21: There are duplicate "### Added" headings; move the entry
"✨(frontend) add the presenter mode" into the existing "### Added" section (the
first "### Added" under the [Unreleased] header) instead of creating a second
"### Added" block, ensuring only one "### Added" subsection exists and that the
moved bullet becomes part of that list.
In `@src/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.ts`:
- Around line 58-64: The test currently uses a fixed sleep
(page.waitForTimeout(200)) after calling document.exitFullscreen(), which is
flaky; replace that sleep with a deterministic wait that observes fullscreen
state instead. After the page.evaluate block that checks
document.fullscreenElement and calls document.exitFullscreen(), remove
page.waitForTimeout and add a wait using Playwright's waiting primitive (e.g.,
page.waitForFunction or equivalent) to wait until document.fullscreenElement is
null (or until the app-specific presenter/fullscreen DOM state updates), then
proceed to page.keyboard.press('Escape').
In
`@src/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/useSlides.spec.ts`:
- Around line 5-15: The test helper factories para, heading, divider, and image
currently rely on inferred return types; add explicit return type annotations
(e.g., para(): Block, heading(): Block, divider(): Block, image(): Block) and
import the Block type used by the app's document model so each helper is
declared to return Block to ensure the mock objects conform to the expected
interface.
In
`@src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx`:
- Around line 67-75: When useSlides(snapshotBlocks) can return an empty array,
normalize slides to ensure at least one fallback empty slide before deriving
total/currentIndex/clamp to avoid invalid "1 / 0" presenter state; replace usage
of slides with a normalizedSlides (or reassign slides) that is slides.length ?
slides : [emptySlidePlaceholder], then compute total = normalizedSlides.length
and use normalizedSlides wherever slides was used (affecting useSlides result
handling, currentIndex state logic, clamp callback, and any rendering that reads
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 429763d2-80bd-4dd4-94f1-9ff2d2da0476
⛔ Files ignored due to path filters (1)
src/frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
.gitignoreCHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.tssrc/frontend/apps/impress/src/components/modal/SideModal.tsxsrc/frontend/apps/impress/src/features/docs/doc-header/components/DocToolBox.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/useBrowserFullscreen.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/useFitScale.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/usePresenterShortcuts.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/useSlides.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterFloatingBar.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterSlide.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/constants.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/useBrowserFullscreen.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/useFitScale.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/usePresenterShortcuts.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/useSlides.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/index.ts
| await page.evaluate(async () => { | ||
| if (document.fullscreenElement) { | ||
| await document.exitFullscreen(); | ||
| } | ||
| }); | ||
| await page.waitForTimeout(200); | ||
| await page.keyboard.press('Escape'); |
There was a problem hiding this comment.
Replace fixed 200ms sleep with a deterministic fullscreen/state wait.
waitForTimeout(200) is flaky in CI and can race with React state updates. Wait for fullscreen exit/state observable condition instead.
💡 Suggested fix
await page.evaluate(async () => {
if (document.fullscreenElement) {
await document.exitFullscreen();
}
});
- await page.waitForTimeout(200);
+ await page.waitForFunction(() => document.fullscreenElement === null);
+ await expect(
+ overlay.getByRole('button', { name: 'Enter fullscreen' }),
+ ).toBeVisible();
await page.keyboard.press('Escape');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await page.evaluate(async () => { | |
| if (document.fullscreenElement) { | |
| await document.exitFullscreen(); | |
| } | |
| }); | |
| await page.waitForTimeout(200); | |
| await page.keyboard.press('Escape'); | |
| await page.evaluate(async () => { | |
| if (document.fullscreenElement) { | |
| await document.exitFullscreen(); | |
| } | |
| }); | |
| await page.waitForFunction(() => document.fullscreenElement === null); | |
| await expect( | |
| overlay.getByRole('button', { name: 'Enter fullscreen' }), | |
| ).toBeVisible(); | |
| await page.keyboard.press('Escape'); |
🤖 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/e2e/__tests__/app-impress/presenter-mode.spec.ts` around
lines 58 - 64, The test currently uses a fixed sleep (page.waitForTimeout(200))
after calling document.exitFullscreen(), which is flaky; replace that sleep with
a deterministic wait that observes fullscreen state instead. After the
page.evaluate block that checks document.fullscreenElement and calls
document.exitFullscreen(), remove page.waitForTimeout and add a wait using
Playwright's waiting primitive (e.g., page.waitForFunction or equivalent) to
wait until document.fullscreenElement is null (or until the app-specific
presenter/fullscreen DOM state updates), then proceed to
page.keyboard.press('Escape').
| const para = (text = 'hello') => ({ | ||
| type: 'paragraph', | ||
| content: text === '' ? [] : [{ type: 'text', text }], | ||
| }); | ||
| const heading = (text = 'Title', level = 1) => ({ | ||
| type: 'heading', | ||
| content: [{ type: 'text', text }], | ||
| props: { level }, | ||
| }); | ||
| const divider = () => ({ type: 'divider' }); | ||
| const image = () => ({ type: 'image', props: { url: 'x' } }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding explicit return type annotations to test helpers.
While TypeScript can infer the return types, adding explicit type annotations (e.g., para(): Block, heading(): Block) improves maintainability and ensures the mock objects conform to the expected Block interface.
🤖 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/__tests__/useSlides.spec.ts`
around lines 5 - 15, The test helper factories para, heading, divider, and image
currently rely on inferred return types; add explicit return type annotations
(e.g., para(): Block, heading(): Block, divider(): Block, image(): Block) and
import the Block type used by the app's document model so each helper is
declared to return Block to ensure the mock objects conform to the expected
interface.
| const slides = useSlides(snapshotBlocks as { type: string }[]); | ||
| const [currentIndex, setCurrentIndex] = useState(0); | ||
|
|
||
| const total = slides.length; | ||
| const clamp = useCallback( | ||
| (i: number) => Math.max(0, Math.min(i, total - 1)), | ||
| [total], | ||
| ); | ||
|
|
There was a problem hiding this comment.
Normalize empty slide sets to avoid invalid 1 / 0 presenter state.
When useSlides(...) returns [], the overlay shows no slide while the toolbar still computes from index=0 and total=0 (1 / 0). Add a fallback single empty slide before deriving total and rendering.
💡 Suggested fix
- const slides = useSlides(snapshotBlocks as { type: string }[]);
+ const slides = useSlides(snapshotBlocks as { type: string }[]);
+ const normalizedSlides =
+ slides.length > 0 ? slides : ([[]] as { type: string }[][]);
const [currentIndex, setCurrentIndex] = useState(0);
- const total = slides.length;
+ const total = normalizedSlides.length;
@@
- {mountedIndices.map((i) => (
+ {mountedIndices.map((i) => (
<PresenterSlide
key={i}
- blocks={slides[i] as unknown[]}
+ blocks={normalizedSlides[i] as unknown[]}
frameRef={frameRef}
isCurrent={i === currentIndex}Also applies to: 109-117, 133-150
🤖 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 67 - 75, When useSlides(snapshotBlocks) can return an empty array,
normalize slides to ensure at least one fallback empty slide before deriving
total/currentIndex/clamp to avoid invalid "1 / 0" presenter state; replace usage
of slides with a normalizedSlides (or reassign slides) that is slides.length ?
slides : [emptySlidePlaceholder], then compute total = normalizedSlides.length
and use normalizedSlides wherever slides was used (affecting useSlides result
handling, currentIndex state logic, clamp callback, and any rendering that reads
slides).
a5d2591 to
8757609
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx (1)
67-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize empty slide sets before computing presenter state.
At Line 70,
totalcan be0whenuseSlides(...)returns no slides, which leads to an invalid1 / 0counter and no mounted slide. Normalize once and use the normalized array fortotaland rendering.💡 Suggested fix
- const slides = useSlides(snapshotBlocks as { type: string }[]); + const slides = useSlides(snapshotBlocks as { type: string }[]); + const normalizedSlides = + slides.length > 0 ? slides : ([[]] as { type: string }[][]); const [currentIndex, setCurrentIndex] = useState(0); - const total = slides.length; + const total = normalizedSlides.length; @@ - blocks={slides[i] as unknown[]} + blocks={normalizedSlides[i] as unknown[]}Also applies to: 133-137, 147-149
🤖 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 67 - 71, The code assumes useSlides(snapshotBlocks) returns a non-empty array; normalize slides by assigning a safeSlides variable (e.g., const safeSlides = slides.length ? slides : [{ type: 'empty' }]) and use safeSlides everywhere instead of slides when computing total, rendering, and in functions like clamp; ensure total is derived from safeSlides.length and that currentIndex is clamped into [0, total-1] via setCurrentIndex(Math.min(Math.max(currentIndex, 0), total - 1)) or in the clamp callback, and update usages in useSlides, total, clamp, currentIndex/setCurrentIndex and any rendering logic (also apply the same normalization to the other presenter state sections noted) so you never divide by zero or mount with an invalid index.
🤖 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.
Duplicate comments:
In
`@src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx`:
- Around line 67-71: The code assumes useSlides(snapshotBlocks) returns a
non-empty array; normalize slides by assigning a safeSlides variable (e.g.,
const safeSlides = slides.length ? slides : [{ type: 'empty' }]) and use
safeSlides everywhere instead of slides when computing total, rendering, and in
functions like clamp; ensure total is derived from safeSlides.length and that
currentIndex is clamped into [0, total-1] via
setCurrentIndex(Math.min(Math.max(currentIndex, 0), total - 1)) or in the clamp
callback, and update usages in useSlides, total, clamp,
currentIndex/setCurrentIndex and any rendering logic (also apply the same
normalization to the other presenter state sections noted) so you never divide
by zero or mount with an invalid index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc034e76-1fc6-4e9d-a20b-0ded7d3aa653
⛔ Files ignored due to path filters (1)
src/frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.tssrc/frontend/apps/impress/src/components/modal/SideModal.tsxsrc/frontend/apps/impress/src/features/docs/doc-header/components/DocToolBox.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/useBrowserFullscreen.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/useFitScale.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/usePresenterShortcuts.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/useSlides.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterFloatingBar.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterSlide.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/constants.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/useBrowserFullscreen.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/useFitScale.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/usePresenterShortcuts.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/useSlides.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/index.ts
PanchoutNathan
force-pushed
the
feature/presenter-mode
branch
from
8757609 to
5698fb1
Compare
Update @gouvfr-lasuite/cunningham-react from 4.2.0 to 4.3.0 and @gouvfr-lasuite/ui-kit from 0.19.10 to 0.20.1, and regenerate the cunningham design tokens accordingly. The new ui-kit drops the "black" font weight and ships rounded form border-radius defaults.
cunningham-react 4.3.0 splits Modal props per variant. Switch AlertModal and SideModal from the now-removed ModalProps alias to the more precise ModalDefaultVariantProps type so the modal wrappers keep type-checking against the new API.
Add a presenter overlay that turns the current document into a slide deck. The editor's blocks are snapshot at open time and split into slides on each divider; navigation is driven by keyboard shortcuts and a floating bar with browser fullscreen support. The overlay is wired to the doc header toolbox via a new "Present" entry, lazy-loaded to keep the editor bundle lean.
Cover the three hooks that drive the presenter overlay: useSlides for divider-based block segmentation, useBrowserFullscreen for the Fullscreen API wrapper, and usePresenterShortcuts for the keyboard navigation bindings.
Add a Playwright spec that opens the presenter overlay from the doc options menu, walks through a multi-slide document built with dividers, and verifies that Escape closes the overlay.
PanchoutNathan
force-pushed
the
feature/presenter-mode
branch
from
5698fb1 to
38d10bd
Compare
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
`@src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx`:
- Around line 92-97: PresenterOverlay currently calls enter() inside useEffect
after the dynamic chunk loads (via next/dynamic ssr:false) which can miss the
browser's transient user activation window and silently swallow errors; update
the logic so the fullscreen request is attempted from the original user click
(before crossing the dynamic-import boundary) or at minimum check
navigator.userActivation.isActive and handle enter()'s Promise rejection in
PresenterOverlay (and log/propagate the error) and implement a short
retry/fallback path if activation has expired; reference the enter() and
exitIfOwned() functions in PresenterOverlay and ensure any swallowed promises
are awaited/handled instead of using void.
In `@src/frontend/apps/impress/src/features/docs/doc-presenter/constants.ts`:
- Line 14: The constant PRESENTER_SLIDE_DESIGN_WIDTH is currently 900 but must
match the presenter contract of 800px; update PRESENTER_SLIDE_DESIGN_WIDTH to
800 and verify any scaling/viewport-fit calculations that reference
PRESENTER_SLIDE_DESIGN_WIDTH (e.g., slide scaling logic or CSS-in-JS that
consumes this constant) remain correct after changing the value so the computed
scale and viewport behavior align with the 800px inner container.
🪄 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: 2e51ab05-8e28-4c5f-b99c-f465e0234dd2
⛔ Files ignored due to path filters (1)
src/frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.tssrc/frontend/apps/impress/src/components/modal/SideModal.tsxsrc/frontend/apps/impress/src/features/docs/doc-header/components/DocToolBox.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/useBrowserFullscreen.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/useFitScale.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/usePresenterShortcuts.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/useSlides.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterFloatingBar.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterSlide.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/constants.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/useBrowserFullscreen.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/useFitScale.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/usePresenterShortcuts.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/useSlides.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/index.ts
| useEffect(() => { | ||
| void enter(); | ||
| return () => { | ||
| void exitIfOwned(); | ||
| }; | ||
| }, [enter, exitIfOwned]); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Element.requestFullscreen require transient user activation, and is that activation preserved across an async dynamic import that resolves before the call?
💡 Result:
Yes, Element.requestFullscreen requires transient user activation [1][2]. Transient user activation is a temporary state granted by the browser when a user performs a direct, meaningful interaction (such as a pointerdown, pointerup, or keydown event) [2][3]. Regarding your question about asynchronous operations: yes, transient user activation is designed to be preserved across asynchronous boundaries, such as an await on a dynamic import, provided the operation happens within the lifetime of the activation [4]. When a user interaction occurs, the browser grants the transient activation state to the window [2][4]. This state persists for a short duration (typically around one second) [4] and is not immediately lost when JavaScript execution yields to the event loop via an await keyword [4]. As long as your call to element.requestFullscreen occurs after the async import resolves—but before the activation timer expires—the browser will consider the request to have been initiated by the user [4]. If the code execution takes too long and the activation window expires, the call will fail [2][4]. You can programmatically check if the activation is still active using navigator.userActivation.isActive [2][3].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Element/requestFullscreen
- 2: https://developer.mozilla.org/en-US/docs/Web/Security/Defenses/User_activation
- 3: https://developer.mozilla.org/en-US/docs/Glossary/Transient_activation
- 4: https://developer.chrome.com/blog/user-activation
Fullscreen activation likely survives the async dynamic import, but first-open latency can still break it
Browsers grant transient user activation for fullscreen and preserve it across async boundaries (e.g., await/dynamic import) as long as requestFullscreen runs before the activation window expires. Since PresenterOverlay calls enter() in useEffect after the next/dynamic (ssr:false) chunk loads (src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx, lines 92-97), first open can still fail in real conditions when the chunk load exceeds that short activation window—and void enter() would swallow any rejection. Check navigator.userActivation.isActive (and/or handle enter() rejections) and consider triggering fullscreen directly from the original click handler before crossing the dynamic-import boundary (or adding a retry/fallback).
🤖 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 92 - 97, PresenterOverlay currently calls enter() inside useEffect
after the dynamic chunk loads (via next/dynamic ssr:false) which can miss the
browser's transient user activation window and silently swallow errors; update
the logic so the fullscreen request is attempted from the original user click
(before crossing the dynamic-import boundary) or at minimum check
navigator.userActivation.isActive and handle enter()'s Promise rejection in
PresenterOverlay (and log/propagate the error) and implement a short
retry/fallback path if activation has expired; reference the enter() and
exitIfOwned() functions in PresenterOverlay and ensure any swallowed promises
are awaited/handled instead of using void.
| * inner wrapper renders at this exact width then `transform: scale(...)` | ||
| * fits it into the available viewport. | ||
| */ | ||
| export const PRESENTER_SLIDE_DESIGN_WIDTH = 900; |
There was a problem hiding this comment.
Align slide design width with the presenter contract (800px).
PRESENTER_SLIDE_DESIGN_WIDTH is set to 900, but the PR objective states scaling is based on an 800px inner container. This mismatch will skew scaling and viewport-fit behavior.
Suggested fix
-export const PRESENTER_SLIDE_DESIGN_WIDTH = 900;
+export const PRESENTER_SLIDE_DESIGN_WIDTH = 800;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const PRESENTER_SLIDE_DESIGN_WIDTH = 900; | |
| export const PRESENTER_SLIDE_DESIGN_WIDTH = 800; |
🤖 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/constants.ts` at
line 14, The constant PRESENTER_SLIDE_DESIGN_WIDTH is currently 900 but must
match the presenter contract of 800px; update PRESENTER_SLIDE_DESIGN_WIDTH to
800 and verify any scaling/viewport-fit calculations that reference
PRESENTER_SLIDE_DESIGN_WIDTH (e.g., slide scaling logic or CSS-in-JS that
consumes this constant) remain correct after changing the value so the computed
scale and viewport behavior align with the 800px inner container.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/frontend/apps/impress/src/features/docs/doc-presenter/constants.ts (1)
14-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSlide design width still mismatches the 800px presenter contract.
PRESENTER_SLIDE_DESIGN_WIDTHremains900, while the PR objective specifies an800pxinner container. This skews the width-based scale (scaleW = availW / designWidth) and viewport-fit behavior. Confirm whether the contract is 800 or 900 and align the constant (and the pinnedOPTS.designWidthinuseFitScale.spec.ts) accordingly.🤖 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/constants.ts` at line 14, PRESENTER_SLIDE_DESIGN_WIDTH is set to 900 but the presenter contract expects 800; update PRESENTER_SLIDE_DESIGN_WIDTH to 800 and make the corresponding change to the pinned OPTS.designWidth in useFitScale.spec.ts so both the runtime constant and the test fixture use the same 800px design width; locate and update the constant export PRESENTER_SLIDE_DESIGN_WIDTH and the OPTS.designWidth reference in useFitScale.spec.ts to keep scaleW = availW / designWidth consistent with the 800px contract.
🤖 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/e2e/__tests__/app-impress/presenter-mode.spec.ts`:
- Around line 186-188: The test hard-codes 900 for the slide design width;
import and use the source-of-truth constant PRESENTER_SLIDE_DESIGN_WIDTH instead
of the literal so the assertion uses PRESENTER_SLIDE_DESIGN_WIDTH * scale;
update the assertion in presenter-mode.spec.ts (where dims.outerWidth is
compared) to reference PRESENTER_SLIDE_DESIGN_WIDTH and ensure the import is
added at the top of the test file.
In
`@src/frontend/apps/impress/src/features/docs/doc-header/components/DocToolBox.tsx`:
- Around line 189-197: The Present icon in DocToolBox.tsx is inconsistent with
other dropdown icons: replace the raw <Present /> used in the menu item (the
object whose callback calls setIsPresenterOpen) with the ui-kit-styled version
used elsewhere — e.g. render Present with width={24} height={24} and
aria-hidden="true" (or use the ui-kit Icon wrapper API if available) so the
dropdown entry matches other icons and is marked decorative for assistive tech;
keep the rest of the menu item (show, testId, callback) unchanged.
In
`@src/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/useSlides.spec.ts`:
- Line 35: The tests use `as any` on mock helpers (e.g., image(), para(),
heading(), divider()) when calling `isEmptyBlock`; remove these casts by
declaring a shared `Block` type that covers all variants used by `isEmptyBlock`
and annotate each helper's return type to `Block` (or the appropriate union
member) so the mocks are type-compatible; then delete the `as any` casts from
the test expectations (references: isEmptyBlock, image, para, heading, divider).
In
`@src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx`:
- Around line 60-65: PresenterOverlay currently only records the prior focus via
addLastFocus in its useEffect but never moves focus into the dialog (it sets
role="dialog" and aria-modal="true" but no focus management), so update
PresenterOverlay to, on mount/open, call addLastFocus(document.activeElement)
then move focus into the dialog container (e.g., set a ref on the dialog
element, ensure it has tabIndex={-1}, and call ref.current.focus()) and ensure
focus is restored on unmount via useFocusStore; alternatively, if you prefer a
trap, integrate a focus-trap (or use an existing useFocusTrap hook) around the
dialog to contain keyboard focus while open—refer to the PresenterOverlay
component, its useEffect, addLastFocus, and useFocusStore when implementing the
change.
---
Duplicate comments:
In `@src/frontend/apps/impress/src/features/docs/doc-presenter/constants.ts`:
- Line 14: PRESENTER_SLIDE_DESIGN_WIDTH is set to 900 but the presenter contract
expects 800; update PRESENTER_SLIDE_DESIGN_WIDTH to 800 and make the
corresponding change to the pinned OPTS.designWidth in useFitScale.spec.ts so
both the runtime constant and the test fixture use the same 800px design width;
locate and update the constant export PRESENTER_SLIDE_DESIGN_WIDTH and the
OPTS.designWidth reference in useFitScale.spec.ts to keep scaleW = availW /
designWidth consistent with the 800px contract.
🪄 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: 24b27351-2ad1-4726-9f46-c447c4b59b28
⛔ Files ignored due to path filters (1)
src/frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.tssrc/frontend/apps/impress/src/components/modal/SideModal.tsxsrc/frontend/apps/impress/src/features/docs/doc-header/components/DocToolBox.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/useBrowserFullscreen.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/useFitScale.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/usePresenterShortcuts.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/__tests__/useSlides.spec.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterFloatingBar.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterSlide.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/constants.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/useBrowserFullscreen.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/useFitScale.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/usePresenterShortcuts.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/useSlides.tssrc/frontend/apps/impress/src/features/docs/doc-presenter/index.ts
| // The core invariant: outer width = DESIGN_WIDTH (900) × scale, | ||
| // within a 5px tolerance for sub-pixel rounding. | ||
| expect(Math.abs(dims.outerWidth - 900 * scale)).toBeLessThan(5); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the design-width constant value used by the scaler.
fd -t f 'constants.ts' --full-path doc-presenter --exec rg -nP 'DESIGN_WIDTH|WINDOW_RADIUS|MIN_SCALE|MAX_SCALE|PADDING' {}
rg -nP 'DESIGN_WIDTH' src/frontend/apps/impress/src/features/docs/doc-presenterRepository: suitenumerique/docs
Length of output: 834
🏁 Script executed:
#!/bin/bash
# Inspect the assertion in the spec and look for 800/900 and constant usage.
sed -n '160,210p' src/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.ts
rg -n "800|900|PRESENTER_SLIDE_DESIGN_WIDTH|DESIGN_WIDTH" src/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.ts
# Show the constant definition to confirm the source of truth.
sed -n '1,80p' src/frontend/apps/impress/src/features/docs/doc-presenter/constants.tsRepository: suitenumerique/docs
Length of output: 3931
Hard-coded 900 matches the source design width, but avoid duplicating the literal. The spec’s invariant uses 900 * scale, and the source-of-truth constant PRESENTER_SLIDE_DESIGN_WIDTH is set to 900, so the assertion isn’t inconsistent with the implementation. Prefer importing PRESENTER_SLIDE_DESIGN_WIDTH in the e2e test instead of duplicating 900 for future-proofing.
🤖 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/e2e/__tests__/app-impress/presenter-mode.spec.ts` around
lines 186 - 188, The test hard-codes 900 for the slide design width; import and
use the source-of-truth constant PRESENTER_SLIDE_DESIGN_WIDTH instead of the
literal so the assertion uses PRESENTER_SLIDE_DESIGN_WIDTH * scale; update the
assertion in presenter-mode.spec.ts (where dims.outerWidth is compared) to
reference PRESENTER_SLIDE_DESIGN_WIDTH and ensure the import is added at the top
of the test file.
| { | ||
| label: t('Present'), | ||
| icon: <Present />, | ||
| callback: () => { | ||
| setIsPresenterOpen(true); | ||
| }, | ||
| show: !doc.deleted_at && !isSmallMobile, | ||
| testId: `docs-actions-present-${doc.id}`, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@gouvfr-lasuite/ui-kit icons component props width height aria-hidden
💡 Result:
I couldn’t find any authoritative, component-level documentation for a dedicated “Icon” component inside @gouvfr-lasuite/ui-kit that explicitly lists props named width, height, or aria-hidden. What I can support from sources: 1) @gouvfr-lasuite/ui-kit does include an “Icon” component (described as “Material icons”), but the sources retrieved here don’t show its prop types or a public API table for width/height/aria-hidden. [1][2] 2) For accessibility, a common and standards-backed rule is that decorative icons should be hidden from the accessibility tree using aria-hidden="true". This behavior is described by MDN: aria-hidden="true" removes the element (and its children) from the accessibility tree. [3] Practical answer (what you should do, given the missing API props evidence): - Use aria-hidden="true" on purely decorative icons (e.g., SVG/Material icon spans used only for decoration), per MDN guidance on aria-hidden. [3] - For width/height: since no @gouvfr-lasuite/ui-kit prop evidence was found, size the icon via CSS (e.g., setting font-size for Material Icons or applying width/height to the underlying SVG/element) or via whatever size prop is actually implemented by the library (not found in the retrieved sources). If you paste the actual TS/React code snippet you’re using with the Icon component (or the Icon component import path, e.g. where Icon is exported from in @gouvfr-lasuite/ui-kit), I can run a more targeted lookup for the exact prop names and confirm whether width/height are supported as component props or only via styling.
Citations:
- 1: https://registry.npmjs.org/%40gouvfr-lasuite%2Fui-kit
- 2: https://registry.npmjs.org/@gouvfr-lasuite/ui-kit
- 3: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-hidden
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the exact surrounding code in DocToolBox.tsx
FILE="src/frontend/apps/impress/src/features/docs/doc-header/components/DocToolBox.tsx"
if [ -f "$FILE" ]; then
echo "=== $FILE (lines around 175-215) ==="
sed -n '165,215p' "$FILE" | nl -ba | sed 's/^/ /'
else
echo "Missing file: $FILE"
fi
# 2) Find the import source for Present and other icons in this file
echo "=== Imports in $FILE (top part) ==="
sed -n '1,120p' "$FILE" | nl -ba | sed 's/^/ /'
# 3) Locate where Present is used/imported elsewhere in the repo
echo "=== Repo-wide search for Present icon usage ==="
rg -n "Present\s*[>,)]|<Present\b|from ['\"]`@gouvfr-lasuite/ui-kit/icons`['\"][^;]*|`@gouvfr-lasuite/ui-kit/icons`" -S .
# 4) If node_modules is present in the sandbox, inspect ui-kit types/implementation for Present
if [ -d "node_modules/@gouvfr-lasuite/ui-kit" ]; then
echo "=== node_modules/@gouvfr-lasuite/ui-kit present types/implementation ==="
# Try to find Present export
rg -n "export (const|function|class) Present|Present\s*:" node_modules/@gouvfr-lasuite/ui-kit -S || true
# Try to find icon component prop types for width/height/aria-hidden
rg -n "aria-hidden|width\s*\?|height\s*\?" node_modules/@gouvfr-lasuite/ui-kit -S || true
else
echo "node_modules/@gouvfr-lasuite/ui-kit not present in this sandbox."
fiRepository: suitenumerique/docs
Length of output: 236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/frontend/apps/impress/src/features/docs/doc-header/components/DocToolBox.tsx"
echo "=== Verify file exists ==="
ls -la "$FILE" 2>/dev/null || { echo "Missing file: $FILE"; exit 1; }
echo "=== $FILE (lines 175-215) ==="
sed -n '175,215p' "$FILE" | cat -n
echo "=== $FILE imports (lines 1-120) ==="
sed -n '1,120p' "$FILE" | cat -n
echo "=== Repo-wide search: where Present is imported/used ==="
rg -n --hidden --glob '!.git/*' --glob '!**/dist/**' --glob '!**/build/**' \
"from ['\"]`@gouvfr-lasuite/ui-kit/icons`['\"][^;]*Present|<Present\b|Present\s*\(" .
echo "=== Repo-wide search: icon size/accessibility patterns used elsewhere ==="
rg -n --hidden --glob '!.git/*' --glob '!**/dist/**' --glob '!**/build/**' \
"width=\{24\}\s*height=\{24\}|aria-hidden=\{?\"true\"\\}?" \
src/frontend/apps/impress/src/features/docs/doc-header/components/DocToolBox.tsx
# Inspect node_modules types/impl if available (best-effort)
if [ -d "node_modules/@gouvfr-lasuite/ui-kit" ]; then
echo "=== node_modules/@gouvfr-lasuite/ui-kit exists; searching for Present ==="
rg -n "Present" node_modules/@gouvfr-lasuite/ui-kit -S | head -n 50 || true
rg -n "export .*Present|Present\s*:" node_modules/@gouvfr-lasuite/ui-kit -S | head -n 50 || true
else
echo "=== node_modules/@gouvfr-lasuite/ui-kit not present ==="
fiRepository: suitenumerique/docs
Length of output: 7833
🌐 Web query:
@gouvfr-lasuite/ui-kit/icons Present width height aria-hidden
💡 Result:
I couldn’t find any authoritative, source-backed documentation stating specific default width/height values for the icons provided by @gouvfr-lasuite/ui-kit/icons. What I can confirm from the official accessibility guidance that relates to the “aria-hidden” part: - For decorative icons, using aria-hidden="true" is recommended; aria-hidden="true" removes the element (and its children) from the accessibility tree [1]. - If an icon conveys meaning or duplicates/overlaps accessible text, aria-hidden should be overridden (or not used) so assistive tech users aren’t misled [1]. So, for the query “Present width height aria-hidden”: - aria-hidden: set aria-hidden="true" when the icon is purely decorative; otherwise provide an accessible name/label and don’t hide it [1]. - width/height: the sources found do not specify a fixed width/height for @gouvfr-lasuite/ui-kit/icons; you likely need to refer to the actual Icon component/props (or the underlying SVG/icon size utilities) in that repository, which weren’t surfaced by the searches I ran. If you share the exact component name or the code snippet you’re using (e.g., the React <Icon...> usage or the SVG markup), I can pinpoint the exact default width/height behavior and whether aria-hidden is set by default in that implementation.
Citations:
🌐 Web query:
site:npmjs.com @gouvfr-lasuite/ui-kit/icons Present
💡 Result:
There is no public npm package named @gouvfr-lasuite/ui-kit/icons. The French State Design System (DSFR - Système de Design de l'État) provides its official icons and pictograms through the primary package @gouvfr/dsfr [1]. In this package, icons are located within the dist/icons directory of the @gouvfr/dsfr distribution [1][2]. To use them, developers typically: 1. Include the necessary CSS files from dist/utility/icons/ (e.g., icons-system.min.css) [1][2]. 2. Reference the icons using standard CSS classes as described in the official DSFR documentation [1][2]. If you are working with specific framework integrations, you may be using community-maintained wrappers such as @codegouvfr/react-dsfr [3] or @betagouv/ngx-dsfr [2], which provide their own methods for handling DSFR assets and components. Please verify your dependency name or consult the official Systeme de Design documentation for the most accurate implementation details.
Citations:
- 1: https://www.npmjs.com/package/@gouvfr/dsfr
- 2: https://www.npmjs.com/package/@betagouv/ngx-dsfr
- 3: https://www.npmjs.com/~codegouv-owner
🌐 Web query:
site:github.com @gouvfr-lasuite/ui-kit Present icon component props width height aria-hidden
💡 Result:
I couldn’t find any public GitHub repository/package named @gouvfr-lasuite/ui-kit, nor any “Present” icon React component exposing width, height, or aria-hidden props.
What is public under the La Suite numérique GitHub org is suitenumerique/cunningham (their design system). Its React package is @gouvfr-lasuite/cunningham-react, and icons are imported as CSS “Material Icons” (not as per-icon React components), e.g. via:
@import "@gouvfr-lasuite/cunningham-react/icons";(github.com)
So, in the public codebase I can access, there isn’t a “Present” icon component with width/height/aria-hidden props to document.
If you share the exact GitHub repo URL (or the file path for the icon component), I can extract the precise props from the source.
Citations:
🌐 Web query:
"export" "Present" "@gouvfr-lasuite/ui-kit" "icons"
💡 Result:
I can’t find (in the sources I retrieved) any documentation or code that matches the exact query phrase “export Present @gouvfr-lasuite/ui-kit icons”. What I can confirm is how icons are provided/used by the La Suite UI Kit package. 1) Icons in @gouvfr-lasuite/ui-kit - The npm package @gouvfr-lasuite/ui-kit includes an Icon component described as “Material icons” [1]. - The examples show importing @gouvfr-lasuite/ui-kit “style” separately, e.g. import "@gouvfr-lasuite/ui-kit/style" [1][2]. 2) Where to look in the upstream repo - The ui-kit GitHub repository has an icon/ folder, which is likely where the icon assets/components live [2]. 3) About “export” and “Present” in your query - In the sources retrieved, “export” appears only in the generic sense of importing modules or exporting dist assets for the separate @gouvfr-lasuite/integration package, not for an “export Present … icons” pattern tied to @gouvfr-lasuite/ui-kit [3][4]. - “Present” is not found as part of an @gouvfr-lasuite/ui-kit icon export/usage specification in the retrieved results. If you paste the exact file/snippet you’re trying to interpret (or the error message you’re seeing), I can do a targeted search for the specific “Present”/“export” usage within the ui-kit codebase (e.g., whether it’s a React component, an icon name, a barrel export in an index.ts, etc.).
Citations:
- 1: https://registry.npmjs.org/%40gouvfr-lasuite%2Fui-kit
- 2: https://github.com/suitenumerique/ui-kit
- 3: https://registry.npmjs.org/@gouvfr-lasuite/integration
- 4: https://integration.lasuite.numerique.gouv.fr/guides/getting-started/
Make the Present dropdown icon consistent (24×24 + decorative a11y)
In DocToolBox.tsx, every other dropdown option icon is rendered with width={24} height={24} aria-hidden="true", but Present is currently icon: <Present />. Update Present so it’s sized to 24×24 and hidden from assistive tech as a decorative icon (using the ui-kit’s supported API or by applying equivalent attributes to the rendered element).
🤖 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-header/components/DocToolBox.tsx`
around lines 189 - 197, The Present icon in DocToolBox.tsx is inconsistent with
other dropdown icons: replace the raw <Present /> used in the menu item (the
object whose callback calls setIsPresenterOpen) with the ui-kit-styled version
used elsewhere — e.g. render Present with width={24} height={24} and
aria-hidden="true" (or use the ui-kit Icon wrapper API if available) so the
dropdown entry matches other icons and is marked decorative for assistive tech;
keep the rest of the menu item (show, testId, callback) unchanged.
| }); | ||
|
|
||
| test('image is never empty', () => { | ||
| expect(isEmptyBlock(image() as any)).toBe(false); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Remove as any type assertions for better type safety.
The as any casts bypass TypeScript's type checking. This suggests the mock helpers don't return types compatible with isEmptyBlock's parameter type. The proper fix is to define or import a Block type (or union of block variants), ensure all helpers return that type, and remove these assertions.
🔧 Suggested approach
- Import or define a
Blocktype that covers all variants (paragraph, heading, divider, image):
type Block =
| { type: 'paragraph'; content: Array<{type: 'text'; text: string}> }
| { type: 'heading'; content: Array<{type: 'text'; text: string}>; props: {level: number} }
| { type: 'divider' }
| { type: 'image'; props: {url: string} };- Add return type annotations to helpers (addressing the past review comment):
const para = (text = 'hello'): Block => ({ /* ... */ });
const heading = (text = 'Title', level = 1): Block => ({ /* ... */ });
const divider = (): Block => ({ type: 'divider' });
const image = (): Block => ({ type: 'image', props: { url: 'x' } });- Remove the
as anycasts from lines 35, 39, and 44.
Also applies to: 39-39, 44-44
🤖 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/__tests__/useSlides.spec.ts`
at line 35, The tests use `as any` on mock helpers (e.g., image(), para(),
heading(), divider()) when calling `isEmptyBlock`; remove these casts by
declaring a shared `Block` type that covers all variants used by `isEmptyBlock`
and annotate each helper's return type to `Block` (or the appropriate union
member) so the mocks are type-compatible; then delete the `as any` casts from
the test expectations (references: isEmptyBlock, image, para, heading, divider).
| useEffect(() => { | ||
| if (typeof document === 'undefined') { | ||
| return; | ||
| } | ||
| addLastFocus(document.activeElement as HTMLElement | null); | ||
| }, [addLastFocus]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts --type=tsx -C3 '\b(focus-?trap|FocusTrap|trapFocus|useFocusTrap)\b'
rg -nP -C3 'aria-modal' --glob '*.tsx'Repository: suitenumerique/docs
Length of output: 1837
🏁 Script executed:
#!/bin/bash
# Search for existing focus-trap/focus-management utilities (case-insensitive)
rg -n -i --glob '*.{ts,tsx,js,jsx}' '\b(focus-?trap|usefocustrap|trapFocus|FocusTrap|useFocusTrap)\b'
# Search for common focus-trap related libraries/usages (case-insensitive)
rg -n -i --glob '*.{ts,tsx,js,jsx}' '\b(focus-trap|focus trap|tabindex="-1"|restore focus|focusin|lastfocus)\b'
# Find the dialog container in PresenterOverlay to see how focus is managed there
rg -n --glob '*.tsx' 'PresenterOverlay\.tsx|Presenter mode|addLastFocus|role="dialog"|aria-modal="true"' src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsxRepository: suitenumerique/docs
Length of output: 297
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx"
echo "---- PresenterOverlay.tsx (relevant sections) ----"
nl -ba "$FILE" | sed -n '1,120p'
nl -ba "$FILE" | sed -n '120,220p'
echo "---- Find useFocusStore definition ----"
rg -n --glob '*.{ts,tsx}' '\buseFocusStore\b' src/frontend/apps/impress | head -n 50
rg -n --glob '*.{ts,tsx}' '\buseFocusStore\b' src/frontend | head -n 50
echo "---- Locate definition file(s) ----"
defs=$(rg -n --glob '*.{ts,tsx}' 'function useFocusStore|const useFocusStore|export function useFocusStore|export const useFocusStore' src/frontend -S || true)
echo "$defs" | head -n 50
# If we found any definition lines, print surrounding code.
echo "---- Print surrounding code for top definition hits ----"
echo "$defs" | head -n 10 | while read -r line; do
path=$(echo "$line" | cut -d: -f1)
lineno=$(echo "$line" | cut -d: -f2)
if [ -f "$path" ]; then
echo "### $path:$lineno"
nl -ba "$path" | sed -n "$((lineno-30)),$((lineno+80))p" | head -n 200
fi
done
echo "---- Search for focus-trap/focus management in useFocusStore related files ----"
# Search within the directory tree where definition seems to be.
# (We’ll just search for "trap" and "focus" keywords globally for now to catch utilities.)
rg -n --glob '*.{ts,tsx}' '\b(trapFocus|focus-?trap|useFocusTrap|FocusTrap)\b' src/frontend --hidden || true
rg -n --glob '*.{ts,tsx}' '\b(tabindex\s*=\s*"-1"|focus\s*\(|focusin|restore focus|lastFocus|addLastFocus)\b' src/frontend --hidden || trueRepository: suitenumerique/docs
Length of output: 175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx"
echo "---- PresenterOverlay.tsx (top: import/props + around useEffect) ----"
sed -n '1,120p' "$FILE" | awk '{printf "%4d:%s\n", NR, $0}'
echo "---- PresenterOverlay.tsx (around dialog markup) ----"
sed -n '110,200p' "$FILE" | awk '{printf "%4d:%s\n", NR+110, $0}'
echo "---- Find useFocusStore definition/import in this file ----"
rg -n --glob '*.{ts,tsx}' '\buseFocusStore\b' "$FILE"
echo "---- Find useFocusStore definition(s) in src/frontend ----"
rg -n --glob '*.{ts,tsx}' '\buseFocusStore\b' src/frontend | head -n 50
echo "---- Locate likely definition file for useFocusStore ----"
defs=$(rg -n --glob '*.{ts,tsx}' -S '(export\s+(const|function)\s+useFocusStore\b|function\s+useFocusStore\b|const\s+useFocusStore\b|useFocusStore\s*=\s*\()' src/frontend || true)
echo "$defs" | head -n 50
first_def=$(echo "$defs" | head -n 1 | awk -F: '{print $1}')
first_def_line=$(echo "$defs" | head -n 1 | awk -F: '{print $2}')
if [ -n "${first_def:-}" ]; then
echo "---- Printing around first useFocusStore definition: $first_def:$first_def_line ----"
start=$((first_def_line-40))
end=$((first_def_line+120))
sed -n "${start},${end}p" "$first_def" | awk -v s="$start" '{printf "%4d:%s\n", NR+s, $0}'
else
echo "No useFocusStore definition matched the patterns; continuing with a keyword search for addLastFocus/add/remove focus."
fi
echo "---- Search addLastFocus implementation/usages ----"
rg -n --glob '*.{ts,tsx}' '\baddLastFocus\b' src/frontend | head -n 50
# If we find addLastFocus implementation, print around it.
add_impl=$(rg -n --glob '*.{ts,tsx}' -S '\b(addLastFocus)\b' src/frontend | head -n 1 || true)
add_path=$(echo "$add_impl" | awk -F: '{print $1}')
add_line=$(echo "$add_impl" | awk -F: '{print $2}')
if [ -n "${add_path:-}" ] && [ -n "${add_line:-}" ]; then
echo "---- Printing around first addLastFocus hit: $add_path:$add_line ----"
start=$((add_line-40))
end=$((add_line+120))
sed -n "${start},${end}p" "$add_path" | awk -v s="$start" '{printf "%4d:%s\n", NR+s, $0}'
fi
echo "---- Search for any focus trap utilities/usages again (broader) ----"
rg -n --glob '*.{ts,tsx}' -i '\b(focus\s*-?\s*trap|usefocustrap|focustrap|tabindex\s*=\s*"-1"|onfocusin|focusin|keydown.*Tab|keydown.*Tab|lastFocus|restore\s*focus|trap\s*focus)\b' src/frontend --hidden || trueRepository: suitenumerique/docs
Length of output: 11970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Basic file sizes
wc -l src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx
wc -l src/frontend/apps/impress/src/stores/useFocusStore.tsx
# 2) PresenterOverlay.tsx focus-related markup/props
echo "---- PresenterOverlay.tsx focus-related lines (search for tabIndex/autoFocus/ref focus/keydown Tab) ----"
rg -n --glob '*.tsx' -i 'tabindex|autofocus|tab\s*trap|trap|restoreFocus|addLastFocus|focus\s*\(|focusin|keydown.*Tab|aria-modal' \
src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx
echo "---- PresenterOverlay.tsx around dialog container (120-200) ----"
sed -n '110,200p' src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx | awk '{printf "%4d:%s\n", NR+109, $0}'
echo "---- PresenterOverlay.tsx useEffect/addLastFocus (45-80) ----"
sed -n '40,90p' src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx | awk '{printf "%4d:%s\n", NR+39, $0}'
# 3) Full useFocusStore implementation (small file expected)
echo "---- useFocusStore.tsx ----"
sed -n '1,220p' src/frontend/apps/impress/src/stores/useFocusStore.tsx | awk '{printf "%4d:%s\n", NR, $0}'
# 4) Search for any focus-trap utilities elsewhere
echo "---- Search for focus-trap utilities by name ----"
rg -n -i --glob '*.{ts,tsx,js,jsx}' '\b(focus-?trap|usefocustrap|trapFocus|FocusTrap)\b' src/frontend/apps/impress/src || true
echo "---- Search for focus-trap patterns (broader) ----"
rg -n -i --glob '*.{ts,tsx,js,jsx}' '(tabindex\s*=\s*"-1"|onKeyDown.*Tab|keydown.*Tab|focus\s*trap|restoreFocus\()' src/frontend/apps/impress/src || true
# 5) Ensure parent/close path calls restoreFocus
echo "---- Where restoreFocus is called (global search in impress app) ----"
rg -n --glob '*.{ts,tsx}' '\brestoreFocus\b' src/frontend/apps/impress/src || trueRepository: suitenumerique/docs
Length of output: 9567
Move initial focus into the PresenterOverlay dialog on open.
PresenterOverlay.tsx sets role="dialog" / aria-modal="true", but the mount effect only records the previously focused element (addLastFocus(document.activeElement)); useFocusStore just stores lastFocusedElement and restores it on close—no focus is moved into the dialog, and no focus-trap is used in this component.
useEffect(() => {
if (typeof document === 'undefined') {
return;
}
addLastFocus(document.activeElement as HTMLElement | null);
}, [addLastFocus]);🤖 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 60 - 65, PresenterOverlay currently only records the prior focus
via addLastFocus in its useEffect but never moves focus into the dialog (it sets
role="dialog" and aria-modal="true" but no focus management), so update
PresenterOverlay to, on mount/open, call addLastFocus(document.activeElement)
then move focus into the dialog container (e.g., set a ref on the dialog
element, ensure it has tabIndex={-1}, and call ref.current.focus()) and ensure
focus is restored on unmount via useFocusStore; alternatively, if you prefer a
trap, integrate a focus-trap (or use an existing useFocusTrap hook) around the
dialog to contain keyboard focus while open—refer to the PresenterOverlay
component, its useEffect, addLastFocus, and useFocusStore when implementing the
change.
|
🚀 Preview will be available at https://2321-docs.ppr-docs.beta.numerique.gouv.fr/ You can use the existing account with these credentials:
You can also create a new account if you want to. Once this Pull Request is merged, the preview will be destroyed. |
| useEffect(() => { | ||
| if (typeof document === 'undefined') { | ||
| return; | ||
| } | ||
| addLastFocus(document.activeElement as HTMLElement | null); | ||
| }, [addLastFocus]); |
There was a problem hiding this comment.
You don't have to add the last focus element here, you can just remove this part I think, it is better to add it onClick on the element that was the trigger, but I think it is done already. The target does not seems correct as well.
| "@babel/helper-plugin-utils" "^7.27.1" | ||
|
|
||
| "@babel/plugin-transform-modules-systemjs@7.29.4", "@babel/plugin-transform-modules-systemjs@^7.27.1": | ||
| "@babel/plugin-transform-modules-systemjs@^7.27.1": |
There was a problem hiding this comment.
We should remove this commit (⬆️(frontend) bump cunningham-react and ui-kit), seems like it has been updated in another PR.
| if (typeof document === 'undefined') { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I don't think all this gard are necessary, useEffect is trigger after render, and all the useCallback are trigger thanks to a user interaction, so document must be defined.
|
|
||
| const cleaned = groups | ||
| .map((group) => group.filter((b) => !isEmptyBlock(b))) | ||
| .filter((group) => group.length > 0); | ||
|
|
||
| return cleaned.length > 0 ? cleaned : [[]]; | ||
| }; |
There was a problem hiding this comment.
Why do you do a cleaning ?
By doing so you potentially loose data.
I can see the interlinks by example are not displayed, if we add new custom block, they will potentially not be displayed because of the cleaning I think.
Same problem with certain break lines, some area are removed:
There was a problem hiding this comment.
Seems quite overengineered.
| <Box | ||
| $direction="row" | ||
| $align="center" | ||
| $css={barCss} | ||
| role="toolbar" | ||
| aria-label={t('Presenter controls')} |
There was a problem hiding this comment.
It could be nice to get the focus here when opening the presenter mode, to have the tab directly focusing the action buttons.
Not sure if it is the best way to do so, maybe you have a opinion @Ovgodd ?
|
@PanchoutNathan @AntoLC do you also have |
3 participants
Purpose
Adds a built-in presenter mode to any document. Press Present in the
doc actions menu and the document becomes a slide deck — splits on each
dividerblock, navigates with←/→SpaceHome/End, toggles browserfullscreen with
F, closes withEsc. Snapshot-based: live edits do notaffect the running presentation.
Each slide is dynamically scaled to fit the viewport (Notion-style
pattern: outer wrapper +
transform: scaleon an 800px inner). Scale isclamped to
[0.7, 1.5]; vertical scroll only kicks in when the contentcan't fit even at min scale, and the slide top is always reachable.
Short slides centre vertically, long slides scroll past a small top
padding to reach the viewport edge. Smooth cross-fade between slides.
To avoid the reviewer having to create a set of slides, here is a markdown file to copy and paste into a docs file to test the mode:
test-slides.md
Demo
Enregistrement.de.l.ecran.2026-05-25.a.17.33.55.mp4