feat: add new setting for encrypted PDFs size limit for previews#3329
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a persisted numeric setting ChangesBlob URL Protocol Support
E2E PDF preview size limit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labelstype: feature 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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: 1
🤖 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/ui/reducers/e2ePdfPreviewSizeLimit.ts`:
- Around line 14-16: The reducer e2ePdfPreviewSizeLimit must validate that any
incoming value is a positive integer before updating state: for the load branch
where you read action.payload.e2ePdfPreviewSizeLimit and for the
SETTINGS_SET_E2E_PDF_PREVIEW_SIZE_LIMIT_CHANGED branch where you return
action.payload, check with Number.isInteger(value) && value > 0 (or equivalent)
and only commit that value; otherwise return the existing state. Update the
branches referencing action.payload.e2ePdfPreviewSizeLimit and the
SETTINGS_SET_E2E_PDF_PREVIEW_SIZE_LIMIT_CHANGED case to perform this validation
and fall back to state on invalid input.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1c6b876-c1e5-4ee1-9fa4-464fbcaedf7f
📒 Files selected for processing (10)
src/app/PersistableValues.tssrc/app/selectors.tssrc/i18n/en.i18n.jsonsrc/servers/preload/api.tssrc/servers/preload/e2ePdfPreviewSizeLimit.tssrc/store/rootReducer.tssrc/ui/actions.tssrc/ui/components/SettingsView/GeneralTab.tsxsrc/ui/components/SettingsView/features/E2ePdfPreviewSizeLimit.tsxsrc/ui/reducers/e2ePdfPreviewSizeLimit.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (windows-latest)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript for all new code in this codebase unless explicitly told otherwise
Use Fuselage components from@rocket.chat/fuselagefor all UI work — only create custom components when Fuselage doesn't provide the needed functionality
Check Theme.d.ts for valid color tokens when working with Fuselage components
Use optional chaining with fallbacks for platform-specific APIs instead of mocks (e.g., process.getuid?.() ?? 1000) to ensure code works across all platforms without requiring mocks
TypeScript code must use strict mode
Use React functional components with hooks instead of class components
Redux actions must follow the FSA (Flux Standard Action) pattern
Use camelCase for file naming
Use PascalCase for component file names (React components)
Write self-documenting code through clear naming — avoid unnecessary comments
Files:
src/ui/reducers/e2ePdfPreviewSizeLimit.tssrc/store/rootReducer.tssrc/app/selectors.tssrc/servers/preload/e2ePdfPreviewSizeLimit.tssrc/ui/components/SettingsView/GeneralTab.tsxsrc/ui/actions.tssrc/app/PersistableValues.tssrc/servers/preload/api.tssrc/ui/components/SettingsView/features/E2ePdfPreviewSizeLimit.tsx
🔇 Additional comments (9)
src/app/PersistableValues.ts (1)
109-115: LGTM!Also applies to: 208-211
src/ui/actions.ts (1)
131-132: LGTM!Also applies to: 271-271
src/store/rootReducer.ts (1)
24-24: LGTM!Also applies to: 115-115
src/app/selectors.ts (1)
86-87: LGTM!src/servers/preload/e2ePdfPreviewSizeLimit.ts (1)
3-4: LGTM!src/servers/preload/api.ts (1)
21-21: LGTM!Also applies to: 53-53, 100-100
src/ui/components/SettingsView/features/E2ePdfPreviewSizeLimit.tsx (1)
1-72: LGTM!src/ui/components/SettingsView/GeneralTab.tsx (1)
5-5: LGTM!Also applies to: 39-39
src/i18n/en.i18n.json (1)
339-342: LGTM!
nazabucciarelli
changed the title
There was a problem hiding this comment.
Pull request overview
Adds an Electron-side setting and bridge method to control the maximum size (in MB) for PDF previews in end-to-end encrypted rooms, and expands the document viewer to accept blob: URLs so decrypted PDF blobs can be rendered in the embedded viewer.
Changes:
- Added a persisted Redux setting (
e2ePdfPreviewSizeLimit, default 10) with UI control in Settings > General. - Exposed the current limit via the preload bridge (
RocketChatDesktop.getE2ePdfPreviewSizeLimit()). - Allowed
blob:protocol in the document viewer open-window IPC handler.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/reducers/e2ePdfPreviewSizeLimit.ts | New reducer for storing the PDF preview size limit and loading it from persisted settings. |
| src/ui/components/SettingsView/GeneralTab.tsx | Wires the new setting component into the General settings tab. |
| src/ui/components/SettingsView/features/E2ePdfPreviewSizeLimit.tsx | New numeric input setting UI for the limit. |
| src/ui/actions.ts | Adds the new UI action type and payload mapping. |
| src/store/rootReducer.ts | Registers the new reducer in the root reducer. |
| src/servers/preload/e2ePdfPreviewSizeLimit.ts | Adds a preload getter to read the setting from Redux safely. |
| src/servers/preload/api.ts | Exposes the getter on the RocketChatDesktop preload API surface. |
| src/i18n/en.i18n.json | Adds EN strings for the setting title/description. |
| src/documentViewer/ipc.ts | Allows blob: URLs for opening the document viewer window. |
| src/app/selectors.ts | Persists the new setting value as part of persistable values. |
| src/app/PersistableValues.ts | Adds migration to introduce e2ePdfPreviewSizeLimit with a default value. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
# Conflicts: # src/app/selectors.ts # src/servers/preload/api.ts # src/ui/components/SettingsView/GeneralTab.tsx
|
Thanks for the contribution — the implementation follows the existing settings patterns closely (mirrors I pushed two commits to this branch to unblock it: 1. Merge of 2.
Verified locally: One ask before merge: could you confirm the blob preview was tested on a packaged build (not just dev)? The blob URL is created in the server webview and consumed by a separate viewer webview — it should resolve via the shared |
The servers/fetch-info tests spun up a real http.createServer on port 3000 and exercised fetchInfo through the global fetch against localhost. On the Windows CI runner this is flaky (listen race + real localhost round-trip), producing intermittent failures in all 4 cases. Replace the real server with a mocked global.fetch that models the same responses, including post-redirect response.url semantics. No real server, no real network. All 4 cases and assertions preserved.
Linux installer download |
macOS installer download |
I just downloaded the .dmg file to test it with the packaged build and it works exactly as on dev mode, so I think we are good to go with it |
Relates to RocketChat/Rocket.Chat#40517
I added ‘blob:’ as a supported protocol in
src/documentViewer/ipc.tsso that the sent blob is served by the PDF viewer in Electron. Also, a new setting for managing the max size for preview of encrypted PDFs was added following other settings as reference.e2ePdfPreviewSizeLimitsetting title and description were approved by product team.Related task: SUP-1022
Summary by CodeRabbit
New Features
Bug Fixes
Tests