feat(pdf-viewer): resolve issue #2911 – added close and back navigation buttons logic #3137
Conversation
|
Warning Rate limit exceeded@vipulgupta28 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughDocumentViewer.tsx adds a Changes
Sequence DiagramsequenceDiagram
participant User
participant BackBtn as "Back Button"
participant CloseBtn as "Close Button"
participant WebView
participant Viewer as "DocumentViewer"
User->>BackBtn: click
alt webview in fullscreen
BackBtn->>WebView: executeJavaScript("exit fullscreen")
WebView-->>BackBtn: fullscreen exited
BackBtn->>Viewer: closeDocumentViewer()
Viewer-->>User: viewer closed
else not fullscreen
BackBtn->>Viewer: closeDocumentViewer()
Viewer-->>User: viewer closed
end
User->>CloseBtn: click
CloseBtn->>WebView: executeJavaScript("exit fullscreen")
WebView-->>CloseBtn: (if applicable) fullscreen exited
CloseBtn->>WebView: set src = "about:blank"
CloseBtn->>Viewer: set documentUrl = "about:blank"
CloseBtn->>Viewer: closeDocumentViewer()
Viewer-->>User: viewer closed
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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
🧹 Nitpick comments (2)
src/ui/components/ServersView/DocumentViewer.tsx (2)
93-118: Verify the back button UX behavior.The back button only exits fullscreen on the first click and requires a second click to actually close the viewer. This might be unexpected for users who click "back" expecting to close the viewer immediately.
Consider whether the intended behavior should be:
- Current: Click 1 → Exit fullscreen, Click 2 → Close viewer
- Alternative: Click 1 → Exit fullscreen AND close viewer in one action
Additionally, this logic could be simplified using async/await:
- onClick={() => { + onClick={async () => { const webviewElement = webviewRef.current; if (!webviewElement) return; - - webviewElement.executeJavaScript(` + + try { + const result = await webviewElement.executeJavaScript(` (async () => { if (document.fullscreenElement) { document.exitFullscreen(); - return "exited-fullscreen"; + return 'exited-fullscreen'; } else { - return "no-fullscreen"; + return 'no-fullscreen'; } })(); - `) - .then((result) => { - if (result === "no-fullscreen") { - closeDocumentViewer(); - } - }) - .catch((err) => console.error("Back button error:", err)); + `); + + if (result === 'no-fullscreen') { + closeDocumentViewer(); + } + } catch (err) { + console.error('Back button error:', err); + } }}
93-118: Consider extracting common fullscreen exit logic.Both the back and close buttons contain similar logic to exit fullscreen mode. This duplication could be extracted into a reusable helper function.
const exitFullscreenIfActive = async ( webviewElement: HTMLWebViewElement | null ): Promise<boolean> => { if (!webviewElement) return false; try { const result = await webviewElement.executeJavaScript(` (async () => { if (document.fullscreenElement) { await document.exitFullscreen(); return true; } return false; })(); `); return result; } catch (err) { console.error('Error exiting fullscreen:', err); return false; } };Then use it in both buttons:
// Back button onClick={async () => { const wasFullscreen = await exitFullscreenIfActive(webviewRef.current); if (!wasFullscreen) { closeDocumentViewer(); } }} // Close button onClick={async () => { await exitFullscreenIfActive(webviewRef.current); if (webviewRef.current) { webviewRef.current.src = 'about:blank'; } setDocumentUrl('about:blank'); closeDocumentViewer(); }}Also applies to: 129-151
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/ui/components/ServersView/DocumentViewer.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Follow existing TypeScript patterns with strict mode enabled
All new code must pass ESLint and TypeScript checks
Avoid unnecessary comments; only add comments for complex logic or non-obvious decisions
Files:
src/ui/components/ServersView/DocumentViewer.tsx
src/ui/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
src/ui/**/*.tsx: Implement React UI with functional components and hooks
Use Fuselage components (Box, Button, TextInput, Modal, etc.) and import from@rocket.chat/fuselageinstead of raw HTML elements
For Fuselage theming, validate tokens againstTheme.d.tsand only use documented values
Name React component files in PascalCase; non-component files should follow camelCase naming
Files:
src/ui/components/ServersView/DocumentViewer.tsx
🧠 Learnings (4)
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/rootWindow.ts : Keep `src/rootWindow.ts` as the renderer entry for the main window UI
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/ui/**/*.tsx : Use Fuselage components (Box, Button, TextInput, Modal, etc.) and import from `rocket.chat/fuselage` instead of raw HTML elements
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/ui/**/*.tsx : Implement React UI with functional components and hooks
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/preload.ts : Keep `src/preload.ts` as the preload script bridging main and renderer
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
🪛 ESLint
src/ui/components/ServersView/DocumentViewer.tsx
[error] 92-93: Delete ⏎·
(prettier/prettier)
[error] 94-94: Replace ··icon="arrow-back" with icon='arrow-back'
(prettier/prettier)
[error] 95-95: Delete ··
(prettier/prettier)
[error] 96-96: Delete ··
(prettier/prettier)
[error] 97-97: Delete ··
(prettier/prettier)
[error] 98-98: Replace ·············· with ⏎··············webviewElement
(prettier/prettier)
[error] 99-99: Replace webviewElement.executeJavaScript( with .executeJavaScript(⏎··················
(prettier/prettier)
[error] 108-108: Insert ⏎················
(prettier/prettier)
[error] 110-110: Replace "no-fullscreen" with 'no-fullscreen'
(prettier/prettier)
[error] 114-114: Replace "Back·button·error:" with 'Back·button·error:'
(prettier/prettier)
[error] 115-115: Replace ·············· with ············
(prettier/prettier)
[error] 116-116: Replace ··············mi="x8" with ············mi='x8'
(prettier/prettier)
[error] 117-117: Replace ··············color={theme·===·"dark"·?·"white"·:·"default" with ············color={theme·===·'dark'·?·'white'·:·'default'
(prettier/prettier)
[error] 118-119: Replace ··/>⏎ with />
(prettier/prettier)
[error] 122-122: Insert ··
(prettier/prettier)
[error] 123-123: Insert ··
(prettier/prettier)
[error] 124-124: Insert ··
(prettier/prettier)
[error] 125-125: Replace ·········· with ············
(prettier/prettier)
[error] 127-127: Insert ··
(prettier/prettier)
[error] 129-129: Insert ··
(prettier/prettier)
[error] 133-133: Delete ··············
(prettier/prettier)
[error] 142-142: Delete ··············
(prettier/prettier)
[error] 143-143: Delete ·
(prettier/prettier)
[error] 145-145: Delete ··············
(prettier/prettier)
[error] 146-146: Delete ·
(prettier/prettier)
[error] 151-152: Delete ⏎············
(prettier/prettier)
🔇 Additional comments (1)
src/ui/components/ServersView/DocumentViewer.tsx (1)
7-14: LGTM! Interface extension is appropriate.The
srcproperty addition toHTMLWebViewElementis necessary for the close button functionality at line 140 and follows proper TypeScript interface augmentation patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/components/ServersView/DocumentViewer.tsx (1)
36-74: Address root cause: fullscreen permission handlers are granting all fullscreen requests.The concern about subsequent PDFs auto-entering fullscreen is valid, but the root issue isn't in the DocumentViewer component alone. The problem originates from two permission handler files that automatically grant fullscreen requests:
src/ui/main/serverView/index.ts:317(case 'fullscreen': callback(true))src/videoCallWindow/ipc.ts:815(same pattern)These allow any fullscreen request to proceed without restriction. The current DocumentViewer implementation only provides manual exit buttons but doesn't prevent automatic fullscreen entry.
To properly fix issue #2911, consider:
- At permission handler level: Check if fullscreen is already active before granting permission, or implement a "fullscreen lock" after user first exits it
- At DocumentViewer level: Add state tracking to prevent fullscreen requests after initial exit (e.g., track exit attempt and deny subsequent requests)
- Optional: Persist user preference to localStorage to survive viewer reopens
♻️ Duplicate comments (2)
src/ui/components/ServersView/DocumentViewer.tsx (2)
130-153: Fix race condition: await fullscreen exit before resetting src.The close button doesn't wait for
exitFullscreen()to complete before resetting the webview src and closing the viewer. This could cause unexpected behavior or errors if the src is changed while fullscreen is still exiting.Make the onClick handler async and await the
executeJavaScript()call:<IconButton icon='cross' - onClick={() => { + onClick={async () => { const webviewElement = webviewRef.current; if (webviewElement) { - webviewElement.executeJavaScript(` + await webviewElement.executeJavaScript(` if (document.fullscreenElement) { document.exitFullscreen(); } `); webviewElement.src = 'about:blank'; } // reset React state setDocumentUrl('about:blank'); // finally close the viewer closeDocumentViewer(); }} mi='x8' color={theme === 'dark' ? 'white' : 'default'} />
92-153: Critical: Fix all prettier formatting violations.The code has extensive formatting issues that violate the project's prettier configuration. These will cause CI/CD checks to fail and must be fixed before merging.
Run the following command to auto-fix all formatting issues:
npm run format # or npx prettier --write src/ui/components/ServersView/DocumentViewer.tsxKey issues to fix:
- Use single quotes consistently (not double quotes)
- Fix indentation (should be 2 spaces)
- Remove extra whitespace and blank lines
- Ensure consistent spacing around operators
As per coding guidelines: "All new code must pass ESLint and TypeScript checks"
🧹 Nitpick comments (1)
src/ui/components/ServersView/DocumentViewer.tsx (1)
122-153: Consider improving layout consistency and accessibility.The current header layout places the back button outside the flex container (lines 92-120) while the title and close button are inside (lines 122-153). This creates an inconsistent visual structure.
Additionally, the icon buttons lack accessibility attributes, and the original issue #2911 mentioned keyboard shortcuts as an expected feature.
Suggested improvements:
- Consistent layout: Wrap all header elements in a single flex container
- Accessibility: Add
aria-labelattributes to icon buttons- Keyboard shortcuts: Consider adding keyboard event handlers (e.g., Escape to exit)
Example:
<Box display='flex' justifyContent='space-between' alignItems='center' width='100%'> <IconButton icon='arrow-back' aria-label='Exit fullscreen or close viewer' onClick={...} mi='x8' color={theme === 'dark' ? 'white' : 'default'} /> <h2>PDF Viewer</h2> <IconButton icon='cross' aria-label='Close PDF viewer' onClick={...} mi='x8' color={theme === 'dark' ? 'white' : 'default'} /> </Box>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/ui/components/ServersView/DocumentViewer.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Follow existing TypeScript patterns with strict mode enabled
All new code must pass ESLint and TypeScript checks
Avoid unnecessary comments; only add comments for complex logic or non-obvious decisions
Files:
src/ui/components/ServersView/DocumentViewer.tsx
src/ui/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
src/ui/**/*.tsx: Implement React UI with functional components and hooks
Use Fuselage components (Box, Button, TextInput, Modal, etc.) and import from@rocket.chat/fuselageinstead of raw HTML elements
For Fuselage theming, validate tokens againstTheme.d.tsand only use documented values
Name React component files in PascalCase; non-component files should follow camelCase naming
Files:
src/ui/components/ServersView/DocumentViewer.tsx
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/rootWindow.ts : Keep `src/rootWindow.ts` as the renderer entry for the main window UI
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/rootWindow.ts : Keep `src/rootWindow.ts` as the renderer entry for the main window UI
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to **/*.{ts,tsx} : All new code must pass ESLint and TypeScript checks
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/ui/**/*.tsx : Use Fuselage components (Box, Button, TextInput, Modal, etc.) and import from `rocket.chat/fuselage` instead of raw HTML elements
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/ui/**/*.tsx : Implement React UI with functional components and hooks
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/preload.ts : Keep `src/preload.ts` as the preload script bridging main and renderer
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
🔇 Additional comments (2)
src/ui/components/ServersView/DocumentViewer.tsx (2)
12-12: LGTM!The
srcproperty addition to theHTMLWebViewElementinterface is necessary for TypeScript to recognize the property assignment on line 141. This is correctly typed and follows the webview API.
92-120: The back button implementation is correct for presentation mode handling, not page navigation.The review comment misidentifies the intended behavior. Based on the codebase analysis:
Actual purpose: The back button handles presentation/fullscreen mode exit, as confirmed by git commit "fix: PDF presenting mode (#2880)"
Implementation is correct:
- First press exits fullscreen (if active)
- Second press closes the viewer
- This is the appropriate flow for a presentation mode
No page navigation exists: A codebase search found no page navigation handlers (previousPage, navigate page, etc.) for the PDF viewer, confirming page navigation was never the intended feature.
The close button (X) uses identical fullscreen exit logic, further supporting this design pattern.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ui/components/ServersView/DocumentViewer.tsx (1)
132-149: Add error handling for executeJavaScript failures.If
executeJavaScriptfails (e.g., webview is in a bad state), the error will be an unhandled promise rejection. While the cleanup code will still execute, logging the error would aid debugging.Apply this diff to add error handling:
onClick={async () => { const webviewElement = webviewRef.current; if (webviewElement) { - await webviewElement.executeJavaScript(` - if (document.fullscreenElement) { - document.exitFullscreen(); - } - `); + try { + await webviewElement.executeJavaScript(` + if (document.fullscreenElement) { + document.exitFullscreen(); + } + `); + } catch (err) { + console.error('Close button fullscreen exit error:', err); + } webviewElement.src = 'about:blank'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/ui/components/ServersView/DocumentViewer.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Follow existing TypeScript patterns with strict mode enabled
All new code must pass ESLint and TypeScript checks
Avoid unnecessary comments; only add comments for complex logic or non-obvious decisions
Files:
src/ui/components/ServersView/DocumentViewer.tsx
src/ui/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
src/ui/**/*.tsx: Implement React UI with functional components and hooks
Use Fuselage components (Box, Button, TextInput, Modal, etc.) and import from@rocket.chat/fuselageinstead of raw HTML elements
For Fuselage theming, validate tokens againstTheme.d.tsand only use documented values
Name React component files in PascalCase; non-component files should follow camelCase naming
Files:
src/ui/components/ServersView/DocumentViewer.tsx
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/rootWindow.ts : Keep `src/rootWindow.ts` as the renderer entry for the main window UI
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/rootWindow.ts : Keep `src/rootWindow.ts` as the renderer entry for the main window UI
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to **/*.{ts,tsx} : All new code must pass ESLint and TypeScript checks
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/ui/**/*.tsx : Use Fuselage components (Box, Button, TextInput, Modal, etc.) and import from `rocket.chat/fuselage` instead of raw HTML elements
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/ui/**/*.tsx : Implement React UI with functional components and hooks
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/preload.ts : Keep `src/preload.ts` as the preload script bridging main and renderer
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
🔇 Additional comments (2)
src/ui/components/ServersView/DocumentViewer.tsx (2)
12-12: LGTM: Type declaration for webview src.The
srcproperty declaration is appropriate and necessary for line 141 where the webview source is programmatically reset.
94-117: Clarify "back navigation" vs "exit/close" semantics.The PR description states this button provides "back navigation to move one page backward within the document," but the implementation exits fullscreen mode or closes the viewer entirely—it doesn't navigate to the previous PDF page.
While this behavior aligns with issue #2911's goal to provide a way to exit Presentation mode, the "back" terminology may confuse users expecting page navigation.
Consider either:
- Renaming to "Exit" or using a different icon if the intent is to close/exit
- Adding a tooltip or comment clarifying this is exit functionality, not page navigation
- Or implementing actual PDF page navigation if that was the original intent
Can you confirm the expected behavior and update the PR description or button semantics accordingly?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/ui/components/ServersView/DocumentViewer.tsx (1)
139-139: Fix trailing whitespace.Line 139 has significant trailing whitespace after the closing brace that violates formatting rules and will fail prettier/eslint checks.
Apply this diff to remove the trailing whitespace:
- } + }As per coding guidelines: "All new code must pass ESLint and TypeScript checks"
🧹 Nitpick comments (1)
src/ui/components/ServersView/DocumentViewer.tsx (1)
92-152: Consider extracting fullscreen exit logic into a helper function.Both the Back button and Close button contain similar logic for exiting fullscreen mode via
executeJavaScript. Extracting this into a reusable helper function would reduce duplication and improve maintainability.Example helper function:
const exitFullscreenIfActive = async ( webviewElement: HTMLWebViewElement | null, returnStatus = false ): Promise<string | void> => { if (!webviewElement) return returnStatus ? 'no-webview' : undefined; try { const result = await webviewElement.executeJavaScript(` (async () => { if (document.fullscreenElement) { document.exitFullscreen(); return "exited-fullscreen"; } return "no-fullscreen"; })(); `); return returnStatus ? result : undefined; } catch (err) { console.error('Fullscreen exit error:', err); return returnStatus ? 'error' : undefined; } };Then use it in both button handlers:
// Back button onClick={async () => { const result = await exitFullscreenIfActive(webviewRef.current, true); if (result === 'no-fullscreen') { closeDocumentViewer(); } }} // Close button onClick={async () => { await exitFullscreenIfActive(webviewRef.current); const webviewElement = webviewRef.current; if (webviewElement) { webviewElement.src = 'about:blank'; } setDocumentUrl('about:blank'); closeDocumentViewer(); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/ui/components/ServersView/DocumentViewer.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Follow existing TypeScript patterns with strict mode enabled
All new code must pass ESLint and TypeScript checks
Avoid unnecessary comments; only add comments for complex logic or non-obvious decisions
Files:
src/ui/components/ServersView/DocumentViewer.tsx
src/ui/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
src/ui/**/*.tsx: Implement React UI with functional components and hooks
Use Fuselage components (Box, Button, TextInput, Modal, etc.) and import from@rocket.chat/fuselageinstead of raw HTML elements
For Fuselage theming, validate tokens againstTheme.d.tsand only use documented values
Name React component files in PascalCase; non-component files should follow camelCase naming
Files:
src/ui/components/ServersView/DocumentViewer.tsx
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/rootWindow.ts : Keep `src/rootWindow.ts` as the renderer entry for the main window UI
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/rootWindow.ts : Keep `src/rootWindow.ts` as the renderer entry for the main window UI
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to **/*.{ts,tsx} : All new code must pass ESLint and TypeScript checks
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/ui/**/*.tsx : Use Fuselage components (Box, Button, TextInput, Modal, etc.) and import from `rocket.chat/fuselage` instead of raw HTML elements
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/ui/**/*.tsx : Implement React UI with functional components and hooks
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/preload.ts : Keep `src/preload.ts` as the preload script bridging main and renderer
Applied to files:
src/ui/components/ServersView/DocumentViewer.tsx
🔇 Additional comments (2)
src/ui/components/ServersView/DocumentViewer.tsx (2)
12-12: LGTM! Interface extension is necessary.The
srcproperty declaration is required to support line 141 where the webview source is reset to'about:blank'. This is a valid TypeScript interface extension.
92-120: Inconsistency: Back button doesn't navigate pages as PR claims.The PR description states this adds "a back navigation button that moves one page backward within the document," but the implementation exits fullscreen mode or closes the viewer instead. The actual behavior (exiting Presentation mode) correctly addresses issue #2911, but the PR description is misleading.
The logic itself is sound: if the webview is in fullscreen, it exits fullscreen; otherwise, it closes the viewer entirely.
Please update the PR description to accurately reflect that the Back button exits Presentation mode (fullscreen) rather than navigating backward through pages.
2 participants
Closes #2911
Enhancement: Improved PDF viewer navigation and control
Introduced two key features to improve user experience:
A close button that exits the PDF viewer completely.
A back navigation button that lets users move one page backward within the document.
Summary by CodeRabbit