-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
fix: make thumbnail generation for images in S3 configurable #5185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a configurable setting to control thumbnail generation for images stored in S3. By default, thumbnails will not be generated for S3 images. Users can enable this feature through a toggle in the memo-related settings UI.
Key changes:
- Added
useThumbnailsForS3Imagesboolean field to workspace memo-related settings (defaults to false) - Modified backend attachment service to check this setting before generating thumbnails for S3 images
- Added UI toggle in settings to control this behavior
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/utils/attachment.ts | Added logic to skip thumbnail requests for S3 images when setting is disabled |
| web/src/types/proto/api/v1/workspace_service.ts | Added TypeScript type definitions and serialization for the new setting |
| web/src/locales/en.json | Added English translation for the setting toggle label |
| web/src/components/Settings/MemoRelatedSettings.tsx | Added UI toggle for controlling S3 thumbnail generation |
| server/router/api/v1/workspace_service.go | Added field mapping between API and store representations |
| server/router/api/v1/attachment_service.go | Added server-side check to skip thumbnail generation for S3 images when disabled |
| proto/store/workspace_setting.proto | Added protobuf field definition for the store layer |
| proto/gen/store/workspace_setting.pb.go | Generated Go code for store protobuf |
| proto/gen/openapi.yaml | Updated OpenAPI specification with new field and formatting changes |
| proto/gen/api/v1/workspace_service.pb.go | Generated Go code for API protobuf |
| proto/gen/api/v1/user_service.pb.go | Removed extraneous empty comment line |
| proto/api/v1/workspace_service.proto | Added protobuf field definition for the API layer |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
An alternative implementation would be to introduce a 3-way or 4-way setting (no thumbnails, on-the-fly, stored in S3) stored in the S3 configuration with the following semantics:
This would be the cleanest change in my opinion, but is a larger rework and requires a database migration. What do you think? |
If just solving the issue of excessive memory usage, we can first just add a boolean field in |
|
I've refactored the pull request such that the setting is now located in the storage configuration. Note that the current value is reflected in the frontend attachment object, because no access to the storage configuration is possible for non-admin users. |
6b0dbd9 to
d009c1d
Compare
…ages are stored in S3
… related settings
d009c1d to
1aa30ec
Compare
|
I've rebased the pull request to the current main to fix merge conflicts. |
…o return thumbnails for S3 images
This pull request adds a toggle to disable thumbnails for images stored in S3. The default value is to not generate thumbnails.
Fixes #5183