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:
📝 WalkthroughWalkthroughAdds a reusable Vue CKEditor 5 component with configurable plugins, toolbar, read-only state, uploads, asset browsing, insertion, cleanup, and progress tracking. Symfony endpoints now delegate image uploads and asset listing to the REST uploads client through DTOs and validation. Existing field integration, frontend/backend tests, Vitest discovery, Composer requirements, and documentation are updated. Authentication and authorization exception handling is narrowed in two controllers. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Editor as CkEditor.vue
participant Adapter as EditorUploadAdapter
participant Controller as EditorUploadController
participant Service as EditorUploadService
participant API as RestApiUploadsClient
Editor->>Adapter: Upload selected image
Adapter->>Controller: POST /editor/upload
Controller->>Service: storeImage(uploadedFile)
Service->>API: upload(file, uploadimages)
API-->>Service: Upload result
Service-->>Controller: EditorUploadResult
Controller-->>Adapter: JSON URL and filename
Adapter-->>Editor: CKEditor upload result
sequenceDiagram
participant Editor as CkEditor.vue
participant Picker as EditorAssetPicker
participant Controller as EditorUploadController
participant Service as EditorUploadService
participant API as RestApiUploadsClient
Editor->>Picker: Open asset picker
Editor->>Controller: GET /editor/assets
Controller->>Service: listAssets()
Service->>API: getUploads(uploadimages)
API-->>Service: Asset files
Service-->>Controller: EditorAssetItem list
Controller-->>Editor: JSON asset items
Editor->>Picker: Display and filter assets
Picker-->>Editor: Selected asset
Editor->>Editor: Insert image or linked text
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (6)
tests/Integration/Controller/EditorUploadControllerTest.php (1)
70-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid reusing
$uploadedFilevariable name for a file path.Line 70 reassigns
$uploadedFile(previously anUploadedFileobject) to a string path. This shadowing is confusing and could lead to bugs if the original object is referenced later. Use a distinct variable name.♻️ Proposed refactor: rename variable
- $uploadedFile = $projectDir . '/public/uploadimages/ckeditor5/' . $payload['fileName']; - if (is_file($uploadedFile)) { - unlink($uploadedFile); + $uploadedFilePath = $projectDir . '/public/uploadimages/ckeditor5/' . $payload['fileName']; + if (is_file($uploadedFilePath)) { + unlink($uploadedFilePath); }🤖 Prompt for 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. In `@tests/Integration/Controller/EditorUploadControllerTest.php` around lines 70 - 73, The test cleanup code is reusing $uploadedFile for a string path after it already referred to an UploadedFile object, which is confusing and can lead to accidental misuse. In EditorUploadControllerTest, rename the path variable to something distinct like an uploaded file path name and keep $uploadedFile reserved for the UploadedFile instance, updating the is_file/unlink cleanup block accordingly.src/Service/EditorUploadService.php (1)
91-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten visibility of internal helper methods.
buildRelativeUrl()andgetTargetDirectory()are public but only used within the service and its tests. Making them private reduces the public API surface and prevents accidental misuse.♻️ Proposed refactor: make helpers private
- public function buildRelativeUrl(string $fileName): string + private function buildRelativeUrl(string $fileName): string { return sprintf( '/%s/%s/%s', trim($this->editorImagesDir, '/'), self::STORAGE_SUBDIRECTORY, ltrim($fileName, '/') ); } - public function getTargetDirectory(): string + private function getTargetDirectory(): string { return rtrim($this->projectDir, '/') . '/public/' . trim($this->editorImagesDir, '/') . '/' . self::STORAGE_SUBDIRECTORY; }🤖 Prompt for 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. In `@src/Service/EditorUploadService.php` around lines 91 - 108, Both EditorUploadService helper methods are too broadly exposed for internal-only behavior. Change buildRelativeUrl() and getTargetDirectory() in EditorUploadService to private, and update any direct test usage to exercise them through the service’s public methods or adjust test setup as needed so the internal helpers are no longer part of the public API.tests/Unit/Service/EditorUploadServiceTest.php (1)
79-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared
removePathhelper to a trait or base class.The
removePath()method is duplicated verbatim inEditorUploadControllerTest.php. Extracting it to a shared trait or test base class would eliminate the duplication.♻️ Proposed refactor: extract to trait
+<?php + +declare(strict_types=1); + +namespace PhpList\WebFrontend\Tests\Support; + +trait TempDirCleanupTrait +{ + private function removePath(string $path): void + { + if (is_file($path) || is_link($path)) { + unlink($path); + return; + } + + if (!is_dir($path)) { + return; + } + + foreach (scandir($path) as $item) { + if ($item === '.' || $item === '..') { + continue; + } + $this->removePath($path . DIRECTORY_SEPARATOR . $item); + } + + rmdir($path); + } +}Then in both test files:
+use PhpList\WebFrontend\Tests\Support\TempDirCleanupTrait; + final class EditorUploadServiceTest extends TestCase { + use TempDirCleanupTrait; + - private function removePath(string $path): void - { - // ... duplicated method body - } }🤖 Prompt for 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. In `@tests/Unit/Service/EditorUploadServiceTest.php` around lines 79 - 100, The removePath() helper is duplicated in EditorUploadServiceTest and EditorUploadControllerTest, so extract it into a shared test trait or base class and reuse that single implementation from both tests. Move the recursive filesystem cleanup logic into the shared helper, then update each test class to import or extend it so the duplicated private method can be removed.assets/editor/CkEditor.vue (1)
177-200: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the asset-loading fetch.
loadAssetshas no timeout orAbortController. If the backend is slow or unresponsive,assetLoadingstaystrueindefinitely and the picker shows a perpetual spinner with no recourse for the user.♻️ Proposed refactor with AbortController
const loadAssets = async () => { assetLoading.value = true; assetError.value = ''; + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 15000); try { const response = await fetch(props.assetsEndpoint, { headers: { 'X-Requested-With': 'XMLHttpRequest', }, credentials: props.withCredentials ? 'include' : 'same-origin', + signal: controller.signal, }); if (!response.ok) { throw new Error(`Failed to load assets (${response.status})`); } const payload = await response.json(); assetItems.value = Array.isArray(payload?.items) ? payload.items : []; } catch (error) { - assetError.value = error?.message || 'Failed to load assets.'; + assetError.value = error?.name === 'AbortError' + ? 'Asset request timed out.' + : error?.message || 'Failed to load assets.'; } finally { + clearTimeout(timeoutId); assetLoading.value = false; } };🤖 Prompt for 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. In `@assets/editor/CkEditor.vue` around lines 177 - 200, The loadAssets async flow in CkEditor.vue has no timeout or abort handling, so a slow request can leave assetLoading stuck on and the picker spinning forever. Update loadAssets to use an AbortController (or equivalent timeout mechanism) around the fetch call, cancel the request after a reasonable limit, and handle the abort/error case by setting assetError while always clearing assetLoading in finally.tests/Unit/assets/editor/uploadAdapter.spec.js (1)
6-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests cover the happy and error paths correctly.
The XHR mock pattern is clean — storing listeners by event name and allowing manual triggering gives precise control over the async flow. The success test correctly awaits
Promise.resolve()to letloader.filesettle before firingload, and the error test verifies both default endpoint behavior anderror.messageextraction.The main gaps are
abort()(no test calls it or verifiesxhr.abort()is invoked) and theerrorevent (network-level failure path). Adding these would round out coverage for the adapter's failure modes.🤖 Prompt for 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. In `@tests/Unit/assets/editor/uploadAdapter.spec.js` around lines 6 - 78, The EditorUploadAdapter spec covers upload success and backend error response, but it is missing failure-path coverage for abort and network errors. Extend the existing EditorUploadAdapter tests to exercise adapter.abort() and verify that xhr.abort() is called, and add a case that triggers the XHR error event to confirm the adapter rejects correctly on transport-level failure; use the existing sendMock/listeners setup and the adapter.xhr instance so the new assertions stay aligned with the current test pattern.tests/Unit/assets/editor/CkEditor.spec.js (1)
95-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExisting tests cover the core contract well.
V-model sync, config shape (
licenseKey,toolbar,htmlSupport),openAssetPickerexposure, andreadonly→disabledmapping are all verified correctly against the component'seditorConfigcomputed andisDisabledcomputed.The stub's
.readybutton is available but no test triggers it, sohandleReadyside effects — upload adapter installation, readonly sync, andeditorRefassignment — remain untested. Consider adding a case that clicks.readyand assertsinstallUploadAdapterwas called (e.g., by spying oneditor.plugins.get('FileRepository').createUploadAdapter) and thateditorRefis populated forinsertAssetuse.🤖 Prompt for 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. In `@tests/Unit/assets/editor/CkEditor.spec.js` around lines 95 - 140, The CkEditor tests cover props and v-model, but they do not exercise the handleReady path exposed by the stub’s ready control. Add a test that clicks the .ready button on the mounted CkEditor wrapper and verify the handleReady side effects: editorRef is set, readonly state is synced, and installUploadAdapter runs by spying on editor.plugins.get('FileRepository').createUploadAdapter or the equivalent upload adapter hook.
🤖 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 `@assets/editor/EditorAssetPicker.vue`:
- Around line 55-110: The filtered asset table in EditorAssetPicker.vue has no
empty-state, so when filteredItems is empty it shows only the header row and
looks broken. Update the table rendering around the v-for on filteredItems to
conditionally show a single full-width tbody row with a “No assets found”
message when there are no results, while keeping the existing item rows and
select button behavior intact.
- Around line 1-9: The modal in EditorAssetPicker.vue already has the dialog
semantics but lacks keyboard dismissal and focus handling. Add an Escape-key
handler on the overlay div that closes the picker when open, and in the <script
setup> for EditorAssetPicker, wire basic focus management on open so keyboard
users land inside the dialog instead of needing to tab to the Close button.
In `@src/Service/EditorUploadService.php`:
- Around line 128-139: Block SVG handling in EditorUploadService to prevent
stored XSS: guessExtensionFromMime currently maps image/svg+xml to svg, and
storeImage accepts any image/* MIME, so SVGs can be uploaded and served inline.
Update storeImage to explicitly reject image/svg+xml before extension guessing,
and remove or bypass the svg case in guessExtensionFromMime so only safe raster
formats are stored.
- Around line 26-47: The storeImage method in EditorUploadService currently
validates only upload status and MIME type, but it never enforces an
application-level size limit. Add a file-size check near the start of
storeImage, before any filesystem work or move call, and reject oversized
uploads by throwing a RuntimeException with a clear message. Use the existing
UploadedFile object and keep the new limit consistent with the upload policy
used by this service.
---
Nitpick comments:
In `@assets/editor/CkEditor.vue`:
- Around line 177-200: The loadAssets async flow in CkEditor.vue has no timeout
or abort handling, so a slow request can leave assetLoading stuck on and the
picker spinning forever. Update loadAssets to use an AbortController (or
equivalent timeout mechanism) around the fetch call, cancel the request after a
reasonable limit, and handle the abort/error case by setting assetError while
always clearing assetLoading in finally.
In `@src/Service/EditorUploadService.php`:
- Around line 91-108: Both EditorUploadService helper methods are too broadly
exposed for internal-only behavior. Change buildRelativeUrl() and
getTargetDirectory() in EditorUploadService to private, and update any direct
test usage to exercise them through the service’s public methods or adjust test
setup as needed so the internal helpers are no longer part of the public API.
In `@tests/Integration/Controller/EditorUploadControllerTest.php`:
- Around line 70-73: The test cleanup code is reusing $uploadedFile for a string
path after it already referred to an UploadedFile object, which is confusing and
can lead to accidental misuse. In EditorUploadControllerTest, rename the path
variable to something distinct like an uploaded file path name and keep
$uploadedFile reserved for the UploadedFile instance, updating the
is_file/unlink cleanup block accordingly.
In `@tests/Unit/assets/editor/CkEditor.spec.js`:
- Around line 95-140: The CkEditor tests cover props and v-model, but they do
not exercise the handleReady path exposed by the stub’s ready control. Add a
test that clicks the .ready button on the mounted CkEditor wrapper and verify
the handleReady side effects: editorRef is set, readonly state is synced, and
installUploadAdapter runs by spying on
editor.plugins.get('FileRepository').createUploadAdapter or the equivalent
upload adapter hook.
In `@tests/Unit/assets/editor/uploadAdapter.spec.js`:
- Around line 6-78: The EditorUploadAdapter spec covers upload success and
backend error response, but it is missing failure-path coverage for abort and
network errors. Extend the existing EditorUploadAdapter tests to exercise
adapter.abort() and verify that xhr.abort() is called, and add a case that
triggers the XHR error event to confirm the adapter rejects correctly on
transport-level failure; use the existing sendMock/listeners setup and the
adapter.xhr instance so the new assertions stay aligned with the current test
pattern.
In `@tests/Unit/Service/EditorUploadServiceTest.php`:
- Around line 79-100: The removePath() helper is duplicated in
EditorUploadServiceTest and EditorUploadControllerTest, so extract it into a
shared test trait or base class and reuse that single implementation from both
tests. Move the recursive filesystem cleanup logic into the shared helper, then
update each test class to import or extend it so the duplicated private method
can be removed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 98d609e8-1d6e-46fd-ade2-82794eb5b2f3
📒 Files selected for processing (21)
README.mdassets/editor/CkEditor.vueassets/editor/EditorAssetPicker.vueassets/editor/assetBrowserPlugin.jsassets/editor/index.tsassets/editor/plugins.tsassets/editor/toolbar.tsassets/editor/uploadAdapter.tsassets/vue/components/base/CkEditorField.vuecomposer.jsonconfig/services.ymlsrc/Controller/EditorUploadController.phpsrc/Dto/EditorAssetItem.phpsrc/Dto/EditorUploadResult.phpsrc/Service/EditorUploadService.phptests/Integration/Controller/EditorUploadControllerTest.phptests/Unit/Service/EditorUploadServiceTest.phptests/Unit/assets/editor/CkEditor.spec.jstests/Unit/assets/editor/uploadAdapter.spec.jstests/Unit/assets/vue/components/base/CkEditorField.spec.jsvitest.config.mjs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Controller/DashboardController.php (1)
7-7: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winExisting integration test will fail —
AuthenticationExceptionno longer caught.
DashboardControllerTest::testDashboardRendersDashboardErrorWhenAuthFailsthrowsAuthenticationException('Session expired')fromgetDashboardStats()and asserts a 200 response with the error rendered in the template. With this change,AuthenticationExceptionis no longer caught inindex(), so the exception propagates unhandled. Since the test calls$controller->index($request)directly (not through the HTTP kernel), theUnauthorizedSubscriberwon't intercept it either — the test will receive an uncaught exception instead of a 200 response.If the intent is to let
AuthenticationExceptionpropagate to the subscriber for proper 401/redirect handling, the test needs to be updated accordingly (e.g., asserting a 401 or redirect response, or testing through the full kernel). If the intent is to still render the error in-page, the catch should remain.🔧 Options to resolve the test breakage
Option A — Update the test to expect propagation (if subscriber should handle it):
public function testDashboardRendersDashboardErrorWhenAuthFails(): void { self::bootKernel(); $statsClient = $this->createMock(StatisticsClient::class); $statsClient->expects(self::once()) ->method('getDashboardStats') ->willThrowException(new AuthenticationException('Session expired')); $controller = new DashboardController($statsClient); $controller->setContainer(static::getContainer()); $request = Request::create('/'); $session = new Session(new MockArraySessionStorage()); $session->set('auth_token', 'integration-token'); $request->setSession($session); - $response = $controller->index($request); - $content = (string) $response->getContent(); - - self::assertSame(200, $response->getStatusCode()); - self::assertStringContainsString('data-dashboard-error="Session&`#x20`;expired"', $content); - self::assertStringContainsString('data-dashboard-stats="&`#x5B`;&`#x5B`;"', $content); + $this->expectException(AuthenticationException::class); + $controller->index($request); }Option B — Restore the
AuthenticationExceptioncatch if in-page error rendering is still desired:- } catch (AuthorizationException $e) { + } catch (AuthenticationException | AuthorizationException $e) {Also applies to: 29-29
🤖 Prompt for 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. In `@src/Controller/DashboardController.php` at line 7, Update DashboardController::index and DashboardControllerTest::testDashboardRendersDashboardErrorWhenAuthFails consistently: either retain AuthenticationException handling so the direct controller test still renders the error with a 200 response, or change the test to exercise the full kernel and assert the UnauthorizedSubscriber’s 401/redirect behavior when the exception propagates.
🤖 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.
Outside diff comments:
In `@src/Controller/DashboardController.php`:
- Line 7: Update DashboardController::index and
DashboardControllerTest::testDashboardRendersDashboardErrorWhenAuthFails
consistently: either retain AuthenticationException handling so the direct
controller test still renders the error with a 200 response, or change the test
to exercise the full kernel and assert the UnauthorizedSubscriber’s 401/redirect
behavior when the exception propagates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c96a5003-67c1-4ef4-b40e-ef8a832d7f80
📒 Files selected for processing (2)
src/Controller/AuthController.phpsrc/Controller/DashboardController.php
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/Controller/DashboardController.php`:
- Line 29: Update DashboardController::index and its authentication-failure
contract consistently: either retain the AuthorizationException catch so the
existing dashboard-error HTTP 200 behavior remains, or remove it and update
testDashboardRendersDashboardErrorWhenAuthFails plus the intended UX to assert
UnauthorizedSubscriber’s redirect or 401 response.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d31e41bc-377e-44b0-b06a-62c6edde6add
📒 Files selected for processing (4)
README.mdconfig/services.ymlsrc/Controller/AuthController.phpsrc/Controller/DashboardController.php
🚧 Files skipped from review as they are similar to previous changes (3)
- config/services.yml
- README.md
- src/Controller/AuthController.php
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/Service/EditorUploadService.php`:
- Around line 53-55: Update the fallback filename handling in
EditorUploadService so filenames used by buildRelativeUrl are URL-encoded,
including the alternate fallback path also used later in the method. Encode only
the basename/filename component, preserving directory or URL separators and the
existing response-provided fileName and url behavior.
- Around line 45-46: Update the exception handling in EditorUploadService to
throw the project’s distinct upstream-service exception instead of
RuntimeException when REST uploads fail, preserving the original exception as
the cause. Ensure both controller actions map this upstream failure to HTTP
502/503 while retaining HTTP 400 only for invalid client input, and update the
integration tests to assert the corrected status.
- Around line 137-142: Update the asset construction in EditorUploadService to
handle REST entries missing path or size without undefined-key warnings.
Validate required fields before creating EditorAssetItem, either supplying safe
fallback values or skipping malformed entries, while preserving the existing
defaults for modifiedAt and normal complete entries.
- Line 147: Update loadItems() to sort the mapped assets newest-first before
returning the $items collection, while preserving the existing API field mapping
and return 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc38f4b4-8ebc-4d52-8312-98a498b77799
📒 Files selected for processing (4)
src/Controller/EditorUploadController.phpsrc/Service/EditorUploadService.phptests/Integration/Controller/EditorUploadControllerTest.phptests/Unit/Service/EditorUploadServiceTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Controller/EditorUploadController.php
… response status codes
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Service/EditorUploadService.php (1)
185-188: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBlock SVG uploads to prevent stored XSS.
Hey! Just a quick heads-up: our current check allows
image/svg+xmlto slip through. This is pretty risky since SVGs can run embedded JavaScript in the browser, opening us up to a classic stored XSS vector. Let's explicitly reject SVGs to keep our app secure and chill.🛡️ Proposed fix
$mimeType = (string) $uploadedFile->getMimeType(); - if (!str_starts_with($mimeType, 'image/')) { + if (!str_starts_with($mimeType, 'image/') || $mimeType === 'image/svg+xml') { throw new RuntimeException('Only image uploads are supported.'); }🤖 Prompt for 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. In `@src/Service/EditorUploadService.php` around lines 185 - 188, The image MIME validation in the upload flow should explicitly reject SVG files, including image/svg+xml, before accepting the upload. Update the MIME check in EditorUploadService around $mimeType and preserve acceptance of other image types while throwing the existing RuntimeException for SVG uploads.
♻️ Duplicate comments (1)
src/Service/EditorUploadService.php (1)
140-147: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle incomplete REST asset entries without warnings.
Hey there! Looks like we missed adding safe fallbacks for
pathandsizein our asset list mapping. If those keys are missing from the API response, PHP will throw "undefined array key" warnings that could totally crash our JSON response. Let's add some default fallbacks here to keep everything running smoothly.🛠️ Proposed fix
$items[] = new EditorAssetItem( fileName: $fileName, - url: (string) $file['path'], + url: (string) ($file['path'] ?? $this->buildRelativeUrl($fileName)), mimeType: $mimeType, - size: (int) ($file['size']), + size: (int) ($file['size'] ?? 0), modifiedAt: (int) ($file['modified'] ?? time()), isImage: str_starts_with($mimeType, 'image/'), );🤖 Prompt for 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. In `@src/Service/EditorUploadService.php` around lines 140 - 147, Update the asset mapping in EditorUploadService to safely read missing path and size keys from each $file entry, using appropriate defaults that avoid undefined array key warnings. Preserve the existing type casts and EditorAssetItem construction, and leave the other field fallbacks unchanged.
🧹 Nitpick comments (1)
tests/Unit/Service/EditorUploadServiceTest.php (1)
70-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider mocking
UploadedFileto prevent temp directory bloat.Writing a 10 MB file to disk during test execution can slightly slow things down and leaves a large artifact in the temp directory since the file isn't explicitly removed.
You can keep things snappier and cleaner by mocking
UploadedFileto simulate the size check, which bypasses the disk write entirely!🛠️ Mocking the upload
public function testStoreImageRejectsFilesExceedingMaxSize(): void { $uploadsClient = $this->createMock(UploadsClient::class); $uploadsClient->expects(self::never())->method('upload'); - $sourceFile = tempnam(sys_get_temp_dir(), 'editor-upload-'); - self::assertIsString($sourceFile); - // Write just over the 10 MB limit so the size check trips before upload. - file_put_contents($sourceFile, str_repeat("\0", 10 * 1024 * 1024 + 1)); - $upload = new UploadedFile($sourceFile, 'huge.png', 'image/png', null, true); + $upload = $this->createMock(UploadedFile::class); + $upload->method('isValid')->willReturn(true); + $upload->method('getSize')->willReturn(10 * 1024 * 1024 + 1); $service = new EditorUploadService($uploadsClient); $this->expectException(RuntimeException::class); $this->expectExceptionMessage('The uploaded file exceeds the maximum allowed size of 10 MB.'); $service->storeImage($upload); }🤖 Prompt for 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. In `@tests/Unit/Service/EditorUploadServiceTest.php` around lines 70 - 88, Update testStoreImageRejectsFilesExceedingMaxSize to mock UploadedFile and simulate a size greater than the 10 MB limit through the methods EditorUploadService uses for validation, removing the temp-file creation and file_put_contents setup while preserving the expected exception and the assertion that UploadsClient::upload is never called.
🤖 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.
Outside diff comments:
In `@src/Service/EditorUploadService.php`:
- Around line 185-188: The image MIME validation in the upload flow should
explicitly reject SVG files, including image/svg+xml, before accepting the
upload. Update the MIME check in EditorUploadService around $mimeType and
preserve acceptance of other image types while throwing the existing
RuntimeException for SVG uploads.
---
Duplicate comments:
In `@src/Service/EditorUploadService.php`:
- Around line 140-147: Update the asset mapping in EditorUploadService to safely
read missing path and size keys from each $file entry, using appropriate
defaults that avoid undefined array key warnings. Preserve the existing type
casts and EditorAssetItem construction, and leave the other field fallbacks
unchanged.
---
Nitpick comments:
In `@tests/Unit/Service/EditorUploadServiceTest.php`:
- Around line 70-88: Update testStoreImageRejectsFilesExceedingMaxSize to mock
UploadedFile and simulate a size greater than the 10 MB limit through the
methods EditorUploadService uses for validation, removing the temp-file creation
and file_put_contents setup while preserving the expected exception and the
assertion that UploadsClient::upload is never called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3971e8ef-0883-4fbb-9604-ca153cee19e8
📒 Files selected for processing (12)
assets/editor/EditorAssetPicker.vuesrc/Controller/AuthController.phpsrc/Controller/BaseController.phpsrc/Controller/EditorUploadController.phpsrc/Controller/PublicSubscribeController.phpsrc/EventSubscriber/ApiExceptionSubscriber.phpsrc/Exception/UpstreamServiceException.phpsrc/Service/EditorUploadService.phptests/Integration/Controller/EditorUploadControllerTest.phptests/Unit/Controller/AuthControllerTest.phptests/Unit/EventSubscriber/ApiExceptionSubscriberTest.phptests/Unit/Service/EditorUploadServiceTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- assets/editor/EditorAssetPicker.vue
- src/Controller/AuthController.php
- tests/Integration/Controller/EditorUploadControllerTest.php
2 participants
Summary by CodeRabbit
Summary
Provide a general description of the code changes in your pull request …
were there any bugs you had fixed? If so, mention them. If these bugs have open
GitHub issues, be sure to tag them here as well, to keep the conversation
linked together.
Unit test
Are your changes covered with unit tests, and do they not break anything?
You can run the existing unit tests using this command:
Code style
Have you checked that you code is well-documented and follows the PSR-2 coding
style?
You can check for this using this command:
Other Information
If there's anything else that's important and relevant to your pull
request, mention that information here. This could include benchmarks,
or other information.
If you are updating any of the CHANGELOG files or are asked to update the
CHANGELOG files by reviewers, please add the CHANGELOG entry at the top of the file.
Thanks for contributing to phpList!