refactor(upload): implement dynamic max file upload size configuration and handle upload size exceptions#362
Conversation
…n and handle upload size exceptions
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📜 Recent 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). (3)
📝 WalkthroughWalkthroughConsolidates upload-size configuration into Spring's multipart properties (driven by MAX_UPLOAD_SIZE), removes service-layer size checks, adds a global handler for MaxUploadSizeExceededException (HTTP 413), updates controller to read size from AppSettingService, and surfaces an informational UI message about the env var. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SpringMVC as Server (Spring)
participant AppController as AppUserController
participant AppSetting as AppSettingService
participant UploadService as FileUploadService
participant GlobalHandler as GlobalExceptionHandler
Client->>SpringMVC: POST /upload (multipart)
alt Request size <= spring.servlet.multipart.max-file-size
SpringMVC->>AppController: parsed request with file
AppController->>AppSetting: getMaxUploadSizeMb()
AppController->>UploadService: validateFile(file)
UploadService-->>AppController: validation OK
AppController->>SpringMVC: proceed with upload processing
SpringMVC-->>Client: 200 OK
else Request size > spring.servlet.multipart.max-file-size
SpringMVC--xClient: parsing fails (MaxUploadSizeExceeded)
SpringMVC->>GlobalHandler: MaxUploadSizeExceededException
GlobalHandler-->>Client: 413 Payload Too Large (message with configured MB)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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 unit tests (beta)
✨ Simplify code
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
🧹 Nitpick comments (2)
frontend/src/app/features/settings/global-preferences/global-preferences.component.html (1)
191-194: Good informational message, but consider making the input read-only.The info message explains the environment variable, but the input field and save button remain enabled (lines 173-180). Users can still edit and "save" a value that won't take effect, which may cause confusion.
Consider making the input
[readonly]="true"or[disabled]="true"to clearly indicate it's display-only, with a link or instruction to modify the Docker/environment configuration instead.♻️ Suggested change to make input read-only
<input type="text" pInputText [(ngModel)]="maxFileUploadSizeInMb" + [readonly]="true" [placeholder]="t('fileManagement.maxUploadPlaceholder')"/>And hide or disable the save button for this field.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/settings/global-preferences/global-preferences.component.html` around lines 191 - 194, The environment-variable UI is currently editable even though it’s informational; update global-preferences.component.html so the env-var input element is display-only (add [readonly]="true" or [disabled]="true" on the specific input used for the environment variable) and prevent saving by disabling or hiding the associated save button for that field; ensure you target the input and save button for the env var displayed next to the info-message (the element showing t('fileManagement.envVarNote')) so users see it’s read-only and are guided to change the value via Docker/environment config instead.frontend/src/app/features/settings/global-preferences/global-preferences.component.scss (1)
162-196: Optional: Consider extracting shared base styles.
.warning-messageand.info-messageare nearly identical, differing only in icon color. You could extract a shared%message-baseplaceholder to reduce duplication.♻️ Optional DRY refactor
+%message-base { + display: flex; + align-items: center; + gap: 0.5rem; + font-size: 0.875rem; + color: var(--p-text-muted-color); + margin-top: 0.5rem; + + .pi { + font-size: settings.$settings-section-description-size; + } + + `@media` (width <= 768px) { + margin-top: 0.5rem; + } +} + .warning-message { - display: flex; - align-items: center; - gap: 0.5rem; - font-size: 0.875rem; - color: var(--p-text-muted-color); - margin-top: 0.5rem; + `@extend` %message-base; .pi { color: var(--p-orange-500); - font-size: settings.$settings-section-description-size; - } - - `@media` (width <= 768px) { - margin-top: 0.5rem; } } .info-message { - display: flex; - align-items: center; - gap: 0.5rem; - font-size: 0.875rem; - color: var(--p-text-muted-color); - margin-top: 0.5rem; + `@extend` %message-base; .pi { color: var(--p-blue-500); - font-size: settings.$settings-section-description-size; - } - - `@media` (width <= 768px) { - margin-top: 0.5rem; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/settings/global-preferences/global-preferences.component.scss` around lines 162 - 196, Extract the duplicated rules from .warning-message and .info-message into a shared SCSS placeholder (e.g. %message-base) containing display, align-items, gap, font-size, color, margin-top, and the `@media` block; then change .warning-message and .info-message to `@extend` %message-base and only keep their unique .pi color and font-size override (leave the .pi selector in each to set var(--p-orange-500) and var(--p-blue-500) respectively). This reduces duplication while preserving the existing selectors and responsive behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@booklore-api/src/main/java/org/booklore/service/appsettings/AppSettingService.java`:
- Line 183: The UI allows saving MAX_FILE_UPLOAD_SIZE_IN_MB but
buildAppSettings() (AppSettingService) always uses the Spring-injected
maxUploadSizeMb (builder.maxFileUploadSizeInMb(maxUploadSizeMb)), making saved
values ineffective; fix by either (A) remove
AppSettingKey.MAX_FILE_UPLOAD_SIZE_IN_MB from AppSettingKey and update the
frontend to hide/disable the field and save button for that setting, or (B) keep
the key but make the frontend input read-only/disabled for
AppSettingKey.MAX_FILE_UPLOAD_SIZE_IN_MB (and disable calls to updateSetting()
for it) so the UI reflects that the value is environment-controlled; ensure
AppSettingService.buildAppSettings() continues to use the injected
maxUploadSizeMb and do not attempt to persist or apply the saved value.
---
Nitpick comments:
In
`@frontend/src/app/features/settings/global-preferences/global-preferences.component.html`:
- Around line 191-194: The environment-variable UI is currently editable even
though it’s informational; update global-preferences.component.html so the
env-var input element is display-only (add [readonly]="true" or
[disabled]="true" on the specific input used for the environment variable) and
prevent saving by disabling or hiding the associated save button for that field;
ensure you target the input and save button for the env var displayed next to
the info-message (the element showing t('fileManagement.envVarNote')) so users
see it’s read-only and are guided to change the value via Docker/environment
config instead.
In
`@frontend/src/app/features/settings/global-preferences/global-preferences.component.scss`:
- Around line 162-196: Extract the duplicated rules from .warning-message and
.info-message into a shared SCSS placeholder (e.g. %message-base) containing
display, align-items, gap, font-size, color, margin-top, and the `@media` block;
then change .warning-message and .info-message to `@extend` %message-base and only
keep their unique .pi color and font-size override (leave the .pi selector in
each to set var(--p-orange-500) and var(--p-blue-500) respectively). This
reduces duplication while preserving the existing selectors and responsive
behavior.
🪄 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: f56a2617-9e29-4f70-be0c-12af241168ae
📒 Files selected for processing (9)
booklore-api/src/main/java/org/booklore/app/controller/AppUserController.javabooklore-api/src/main/java/org/booklore/exception/GlobalExceptionHandler.javabooklore-api/src/main/java/org/booklore/service/appsettings/AppSettingService.javabooklore-api/src/main/java/org/booklore/service/upload/FileUploadService.javabooklore-api/src/main/resources/application.yamlbooklore-api/src/test/java/org/booklore/service/upload/FileUploadServiceTest.javafrontend/src/app/features/settings/global-preferences/global-preferences.component.htmlfrontend/src/app/features/settings/global-preferences/global-preferences.component.scssfrontend/src/i18n/en/settings-application.json
💤 Files with no reviewable changes (1)
- booklore-api/src/main/java/org/booklore/service/upload/FileUploadService.java
📜 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). (1)
- GitHub Check: Packaging Smoke Test
🧰 Additional context used
📓 Path-based instructions (4)
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS files
Files:
frontend/src/app/features/settings/global-preferences/global-preferences.component.htmlfrontend/src/app/features/settings/global-preferences/global-preferences.component.scss
frontend/src/i18n/**
📄 CodeRabbit inference engine (AGENTS.md)
Put user-facing strings in Transloco files under
frontend/src/i18n/
Files:
frontend/src/i18n/en/settings-application.json
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/app/controller/AppUserController.javabooklore-api/src/test/java/org/booklore/service/upload/FileUploadServiceTest.javabooklore-api/src/main/java/org/booklore/exception/GlobalExceptionHandler.javabooklore-api/src/main/java/org/booklore/service/appsettings/AppSettingService.java
booklore-api/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required
Files:
booklore-api/src/test/java/org/booklore/service/upload/FileUploadServiceTest.java
🧠 Learnings (3)
📚 Learning: 2026-03-31T14:12:39.521Z
Learnt from: alexhb1
Repo: grimmory-tools/grimmory PR: 307
File: frontend/src/i18n/hr/layout.json:3-3
Timestamp: 2026-03-31T14:12:39.521Z
Learning: In `frontend/src/i18n/hr/layout.json` (Croatian locale), newly added i18n keys may intentionally use English placeholder values (e.g., `toggleNavigation`, `adjustSidebarWidth`, `closeNavigation`). Do not flag these as missing Croatian translations — this is a deliberate placeholder approach by the team.
Applied to files:
frontend/src/i18n/en/settings-application.json
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/test/**/*.java : Prefer focused unit tests; use `SpringBootTest` only when the Spring context is required
Applied to files:
booklore-api/src/test/java/org/booklore/service/upload/FileUploadServiceTest.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*.java : Use MapStruct for entity/DTO mapping
Applied to files:
booklore-api/src/test/java/org/booklore/service/upload/FileUploadServiceTest.java
🔇 Additional comments (8)
booklore-api/src/main/resources/application.yaml (1)
47-48: Solid approach to centralize upload size configuration.Using a single environment variable for both
max-file-sizeandmax-request-sizeensures consistency and directly addresses the root cause of issue#152where the GUI setting wasn't propagating to Tomcat's multipart limits.booklore-api/src/test/java/org/booklore/service/upload/FileUploadServiceTest.java (2)
83-86: Constructor update aligns with refactored service dependencies.The removal of
AppSettingServicefrom the constructor is consistent with the architectural change moving file size validation to Spring's multipart handling.
83-86: FILE_TOO_LARGE is actively used and should not be removed.
FILE_TOO_LARGEis referenced inBookCoverService.java:500and remains in active use. The constant should not be removed.> Likely an incorrect or invalid review comment.booklore-api/src/main/java/org/booklore/exception/GlobalExceptionHandler.java (1)
80-88: Well-implemented exception handler for oversized uploads.The handler correctly:
- Returns HTTP 413 (PAYLOAD_TOO_LARGE) per RFC 7231
- Gracefully handles edge cases where
maxUploadSizeis unknown (≤0)- Provides an informative message with the configured limit
booklore-api/src/main/java/org/booklore/service/appsettings/AppSettingService.java (1)
41-56: Clean injection of Spring's multipart configuration.Using
DataSizefor parsing and converting at startup is efficient. The implementation correctly derives the value from the same property that Spring uses for enforcement.booklore-api/src/main/java/org/booklore/app/controller/AppUserController.java (1)
26-32: Cleaner implementation using centralized service method.Directly calling
appSettingService.getMaxUploadSizeMb()removes the previous error-swallowing try/catch and properly delegates configuration retrieval to the service layer.frontend/src/i18n/en/settings-application.json (1)
35-35: Clear, actionable guidance for users.The message properly explains the environment variable control and the restart requirement, helping users understand how to actually change the limit.
frontend/src/app/features/settings/global-preferences/global-preferences.component.scss (1)
180-196: Consistent styling matching the existing warning message pattern.The
.info-messageclass appropriately uses blue for informational context (vs. orange for warnings). The implementation is consistent with the existing.warning-messagestyling.
zachyale
left a comment
zachyale
left a comment
There was a problem hiding this comment.
We have to think this through better- currently on this branch, the UI pre-fills the value via the ENV var by default, but allows users to edit it and hit save (updating the setting value instead). Refreshing this page then just re-initializes the value in that box to the current ENV var.
I still think we should be highlighting this not as an ENV var, but as a user defined setting, but regardless of whether we pivot to that or keep the ENV var, we need to fix this UI so it makes more sense for whichever direction we go. Discuss tomorrow?
|
As per our maintainer sync up, let's drive this PR to be a full ENV var approach, and freeze this work being merged until after this upcoming release. |
2 participants
Description
Linked Issue: Fixes #152
Changes
This pull request refactors how the maximum file upload size is configured and enforced in the application. The upload size limit is now consistently determined by the
MAX_UPLOAD_SIZEenvironment variable (or the Spring Boot property), and enforced at the framework level, rather than in application logic. Related error handling, configuration, and user interface messaging have been updated to reflect this change.Summary by CodeRabbit
New Features
Bug Fixes
Documentation