feat: make preview conversion timeout and max file size configurable#5408
feat: make preview conversion timeout and max file size configurable#5408juliusknorr wants to merge 1 commit intomainfrom
Conversation
2a2ed3e to
063a515
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements configurable timeout and maximum file size settings for preview conversion operations in the Nextcloud Richdocuments app. The changes address issue #5404 by allowing administrators to customize preview conversion behavior via OCC commands.
Changes:
- Added two new configuration options:
preview_conversion_timeout(default: 5 seconds) andpreview_conversion_max_filesize(default: 100 MB) - Modified the preview generation logic to skip files exceeding the configured size limit
- Updated the conversion service to use the configured timeout for preview requests while maintaining backward compatibility for other conversion operations
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/AppConfig.php | Adds constants and getter methods for the two new configuration options with appropriate defaults |
| lib/Service/RemoteService.php | Adds timeout parameter to convertTo() method and passes configured timeout from convertFileTo() |
| lib/Preview/Office.php | Implements file size check before attempting preview conversion with debug logging |
| docs/app_settings.md | Documents the new configuration options with usage examples |
| tests/lib/AppConfigTest.php | Adds unit tests for the new AppConfig getter methods |
| tests/lib/Preview/OfficeTest.php | Adds unit tests for the new file size checking behavior in preview generation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Julius Knorr <jus@bitgrid.net>
063a515 to
fc9d7c9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $timeout = $this->appConfig->getPreviewConversionTimeout(); | ||
| return $this->convertTo($file->getName(), $stream, $format, [], $timeout); | ||
| } | ||
|
|
There was a problem hiding this comment.
convertFileTo() now unconditionally uses getPreviewConversionTimeout(). This method is also used outside the preview code path (e.g. via ConversionProvider::convertFile()), so changing it makes the preview timeout setting affect non-preview conversions as well, which is inconsistent with the config name/docs and can change behavior unexpectedly.
Consider keeping convertFileTo() on the default timeout and instead passing the preview timeout from Office::getThumbnail() (e.g. by adding an optional timeout parameter to convertFileTo() or introducing a dedicated preview-only method).
| $timeout = $this->appConfig->getPreviewConversionTimeout(); | |
| return $this->convertTo($file->getName(), $stream, $format, [], $timeout); | |
| } | |
| return $this->convertTo($file->getName(), $stream, $format); | |
| } |
| } | ||
| return $this->convertTo($file->getName(), $stream, $format); | ||
| $timeout = $this->appConfig->getPreviewConversionTimeout(); | ||
| return $this->convertTo($file->getName(), $stream, $format, [], $timeout); |
There was a problem hiding this comment.
convertFileTo() opens a local file stream but never closes it. In exception cases (and potentially in long-running PHP processes), this can leak file descriptors.
Wrap the convertTo() call in a try/finally and fclose($stream) in the finally block.
| return $this->convertTo($file->getName(), $stream, $format, [], $timeout); | |
| try { | |
| return $this->convertTo($file->getName(), $stream, $format, [], $timeout); | |
| } finally { | |
| fclose($stream); | |
| } |
| 'preview_conversion_timeout' => 5, | ||
| 'preview_conversion_max_filesize' => 104857600, // 100 MB |
There was a problem hiding this comment.
The new default entries use string literals ('preview_conversion_timeout', 'preview_conversion_max_filesize') even though constants for these keys were introduced just above. Using the constants here would reduce the chance of typos/mismatches if the key names ever change.
| 'preview_conversion_timeout' => 5, | |
| 'preview_conversion_max_filesize' => 104857600, // 100 MB | |
| self::PREVIEW_CONVERSION_TIMEOUT => 5, | |
| self::PREVIEW_CONVERSION_MAX_FILESIZE => 104857600, // 100 MB |
Attempt to implement with Claude Sonnet 4.6 and a custom agent definition, therefore collecting my personal feedback on that a bit here:
Create a branch, Implement the feature described in https://github.com/nextcloud/richdocuments/issues/5404 and open a pull request for itSummary
Fixes #5404
Adds two new configurable options for preview conversion:
Both options can be set via
occ config:app:set:Changes
PREVIEW_CONVERSION_TIMEOUTandPREVIEW_CONVERSION_MAX_FILESIZEconstants + getter methods toAppConfigOffice::getThumbnailnow skips conversion when file size exceeds the configured limitRemoteService::convertFileTopasses the configured timeout to the HTTP clientRemoteService::convertToaccepts a$timeoutparameter (defaults toREMOTE_TIMEOUT_DEFAULTfor non-preview callers)docs/app_settings.md