Conversation
📝 WalkthroughWalkthroughThis pull request introduces a complete template management feature to the application. A new Symfony controller ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/vue/components/templates/TemplateLibrary.vue`:
- Around line 5-15: The New Template, Copy, and Delete buttons in
TemplateLibrary.vue are rendered as active but have no click handlers; either
wire them to actual methods (e.g., createTemplate, copyTemplate, deleteTemplate)
on the component and implement those methods or explicitly disable/hide them
until implemented. Locate the buttons (labels "New Template", "Copy", "Delete")
and add `@click` bindings to the corresponding component methods and ensure those
methods live on the component's methods/options (or Vue setup) with appropriate
behavior, error handling and UI feedback; if you prefer to block UI, add the
disabled attribute (or v-if/v-show) and aria-disabled/tooltip to indicate the
feature is not yet available. Ensure template actions and method names match to
avoid silent no-ops and keep accessibility attributes updated.
- Around line 126-132: loadTemplates currently calls
templateClient.getTemplates(0, 1000) which caps results; update loadTemplates to
iterate over cursor-based pages until pagination.hasMore is false by calling
templateClient.getTemplates with the current cursor (using
pagination.nextCursor) and appending each page's items into templates.value
(instead of replacing it), mirroring the pagination pattern used in
CampaignDirectory.vue and the fetchAllLists helper; also apply the same
cursor-loop fix where CampaignEditView.vue performs its template fetch (around
the getTemplates usage referenced).
In `@assets/vue/views/TemplateEditView.vue`:
- Around line 151-163: templateId is used directly from route.params which can
be malformed; normalize and validate it before any API calls: inside
loadTemplate (and likewise inside the function that performs the PUT), derive a
parsedId from templateId.value (e.g., trim then parseInt or Number), verify
Number.isInteger(parsedId) and parsedId > 0 (or other app-specific constraints),
set loadError.value appropriately and return early if invalid, and pass the
validated parsedId to templateClient.getTemplate and templateClient.put; keep
the computed templateId but do not call the API with its raw value.
- Around line 223-225: The request manually sets the Content-Type header when
sending FormData which breaks boundary handling; in the TemplateEditView.vue
code that calls apiClient.put(`templates/${templateId.value}`, payload, {
'Content-Type': 'multipart/form-data' }), remove the manual Content-Type header
(or the entire third arg if it only contains that header) so the browser/HTTP
client can set multipart/form-data with the correct boundary automatically.
In `@src/Controller/TemplatesController.php`:
- Around line 18-21: The TemplatesController is passing the wrong page title to
the server-rendered SPA; in the render call inside TemplatesController::index
(the call to $this->render('@PhpListFrontend/spa.html.twig', [...])) change the
'page' parameter from 'Campaigns' to the correct template page name (e.g.,
'Templates') so the initial server-rendered title is accurate before Vue
hydrates.
- Around line 25-26: The route for the edit action accepts any path segment but
the controller signature requires an int $templateId, so add a route requirement
to constrain templateId to digits to cause malformed URLs to 404; update the
attribute on the edit method (public function edit(Request $request, int
$templateId): Response) to include requirements={'templateId': '\d+'} or use
Requirement::DIGITS and import Symfony\Component\Routing\Requirement if you
prefer the constant form.
🪄 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: b2518f1c-6619-4b1a-8df0-46daf5170be3
📒 Files selected for processing (5)
assets/router/index.jsassets/vue/components/templates/TemplateLibrary.vueassets/vue/views/TemplateEditView.vueassets/vue/views/TemplatesView.vuesrc/Controller/TemplatesController.php
| <div class="flex gap-2"> | ||
| <button | ||
| class="px-4 py-2 bg-ext-wf1 hover:bg-ext-wf3 text-white text-sm font-medium rounded-lg flex items-center gap-2 transition-colors" | ||
| type="button" | ||
| > | ||
| <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true"> | ||
| <path d="M5 12h14"></path> | ||
| <path d="M12 5v14"></path> | ||
| </svg> | ||
| New Template | ||
| </button> |
There was a problem hiding this comment.
Wire up or hide the inactive action buttons.
New Template, copy, and delete are rendered as enabled buttons, but none has a click handler. Please implement the actions or disable/hide them until supported.
Also applies to: 63-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/templates/TemplateLibrary.vue` around lines 5 - 15, The
New Template, Copy, and Delete buttons in TemplateLibrary.vue are rendered as
active but have no click handlers; either wire them to actual methods (e.g.,
createTemplate, copyTemplate, deleteTemplate) on the component and implement
those methods or explicitly disable/hide them until implemented. Locate the
buttons (labels "New Template", "Copy", "Delete") and add `@click` bindings to the
corresponding component methods and ensure those methods live on the component's
methods/options (or Vue setup) with appropriate behavior, error handling and UI
feedback; if you prefer to block UI, add the disabled attribute (or v-if/v-show)
and aria-disabled/tooltip to indicate the feature is not yet available. Ensure
template actions and method names match to avoid silent no-ops and keep
accessibility attributes updated.
| const loadTemplates = async () => { | ||
| isLoading.value = true | ||
| errorMessage.value = '' | ||
|
|
||
| try { | ||
| const response = await templateClient.getTemplates(0, 1000) | ||
| templates.value = Array.isArray(response?.items) ? response.items : [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect template pagination usage/contracts without modifying the repo.
rg -n -C3 'getTemplates\s*\('
rg -n -C5 'pagination|hasMore|nextCursor'
rg -n -C5 'class TemplatesClient|function getTemplates|getTemplates\s*='Repository: phpList/web-frontend
Length of output: 40146
🏁 Script executed:
rg -n 'templateClient|TemplatesClient' -A5 -B2 assets/vueRepository: phpList/web-frontend
Length of output: 4791
🏁 Script executed:
rg -n 'export.*templateClient|new.*TemplatesClient'Repository: phpList/web-frontend
Length of output: 143
🏁 Script executed:
cat -n assets/vue/api.js | head -100Repository: phpList/web-frontend
Length of output: 2236
Update loadTemplates to paginate through all templates instead of capping at 1000.
The getTemplates(0, 1000) call hard-caps the template library. Since the API supports cursor-based pagination, loop through all pages using pagination.hasMore and pagination.nextCursor, similar to how CampaignDirectory.vue and fetchAllLists handle it in this codebase.
Also affects CampaignEditView.vue line 947.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/templates/TemplateLibrary.vue` around lines 126 - 132,
loadTemplates currently calls templateClient.getTemplates(0, 1000) which caps
results; update loadTemplates to iterate over cursor-based pages until
pagination.hasMore is false by calling templateClient.getTemplates with the
current cursor (using pagination.nextCursor) and appending each page's items
into templates.value (instead of replacing it), mirroring the pagination pattern
used in CampaignDirectory.vue and the fetchAllLists helper; also apply the same
cursor-loop fix where CampaignEditView.vue performs its template fetch (around
the getTemplates usage referenced).
| const templateId = computed(() => route.params.templateId) | ||
|
|
||
| const loadTemplate = async () => { | ||
| if (!templateId.value) { | ||
| loadError.value = 'Template ID is missing.' | ||
| return | ||
| } | ||
|
|
||
| isLoading.value = true | ||
| loadError.value = '' | ||
|
|
||
| try { | ||
| const template = await templateClient.getTemplate(templateId.value) |
There was a problem hiding this comment.
Normalize and validate the route param before API calls.
templateId is used directly from route.params; client-side navigation to a malformed URL can call getTemplate()/put() with a non-numeric ID.
Proposed fix
-const templateId = computed(() => route.params.templateId)
+const templateId = computed(() => {
+ const rawTemplateId = Array.isArray(route.params.templateId)
+ ? route.params.templateId[0]
+ : route.params.templateId
+ const parsedTemplateId = Number(rawTemplateId)
+
+ return Number.isInteger(parsedTemplateId) && parsedTemplateId > 0
+ ? parsedTemplateId
+ : null
+})Also applies to: 195-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/views/TemplateEditView.vue` around lines 151 - 163, templateId is
used directly from route.params which can be malformed; normalize and validate
it before any API calls: inside loadTemplate (and likewise inside the function
that performs the PUT), derive a parsedId from templateId.value (e.g., trim then
parseInt or Number), verify Number.isInteger(parsedId) and parsedId > 0 (or
other app-specific constraints), set loadError.value appropriately and return
early if invalid, and pass the validated parsedId to templateClient.getTemplate
and templateClient.put; keep the computed templateId but do not call the API
with its raw value.
| await apiClient.put(`templates/${templateId.value}`, payload, { | ||
| 'Content-Type': 'multipart/form-data' | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect local usages and available client implementation/type hints without modifying the repo.
rg -n -C4 'new FormData|multipart/form-data|apiClient\.put|\.put\s*\('
rg -n -C5 'class Client|put\s*\('Repository: phpList/web-frontend
Length of output: 4948
🌐 Web query:
Browser FormData manually setting Content-Type multipart/form-data boundary fetch axios
💡 Result:
In browsers, you cannot manually set a custom Content-Type header with a specific boundary when using FormData with fetch or axios. The browser automatically generates and sets the Content-Type to "multipart/form-data; boundary=----WebKitFormBoundary..." (or similar) when FormData is passed as the body. Manually setting the Content-Type header without the correct boundary (which you can't access from standard browser FormData) will cause the request to fail, as the browser won't include the boundary parameter or will ignore your header. Key points from authoritative sources: For fetch (native browser API): - Do NOT set the Content-Type header. Let the browser handle it. - Example: const formData = new FormData; formData.append('file', file); fetch('/upload', { method: 'POST', body: formData }); - Browser auto-sets: Content-Type: multipart/form-data; boundary=----WebKitFormBoundary... For axios (browser): - Same rule applies, as axios uses XMLHttpRequest or fetch under the hood in browsers. - Pass FormData directly; axios detects it and doesn't override the browser's automatic header. - Example: const formData = new FormData; formData.append('file', file); axios.post('/upload', formData); Why manual setting fails: - Browser FormData has no public API to get/set the boundary (confirmed by MDN docs and Stack Overflow consensus). - Setting Content-Type: 'multipart/form-data; boundary=my-custom-boundary' without matching the browser-generated body boundary results in malformed requests. - Workaround for custom boundary: Manually construct the multipart body as a string/Blob (not using FormData), but this loses FormData conveniences like automatic file handling. Official MDN warning (Using FormData Objects): "When using FormData ... do not explicitly set the Content-Type header on the request. Doing so will prevent the browser from being able to set the Content-Type header with the boundary expression it will use to delimit form fields in the request body." This applies to both fetch and XMLHttpRequest (used by axios). Node.js differs (uses form-data library with getBoundary), but query specifies browser context. Code for manual construction (if absolutely needed, not recommended): const boundary = 'my-custom-boundary'; const body = --${boundary}\r\n + 'Content-Disposition: form-data; name="file"; filename="test.txt"\r\n' + 'Content-Type: application/octet-stream\r\n\r\n' + '[binary file data]' + \r\n--${boundary}--; fetch('/upload', { method: 'POST', headers: { 'Content-Type': multipart/form-data; boundary=${boundary} }, body }); But prefer letting the browser handle FormData automatically.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/FormData/Using_FormData_Objects
- 2: https://stackoverflow.com/questions/39280438/fetch-missing-boundary-in-multipart-form-data-post
- 3: https://stackoverflow.com/questions/35192841/how-do-i-post-with-multipart-form-data-using-fetch/35206069
- 4: https://axios-http.com/docs/multipart
- 5: https://stackoverflow.com/questions/11442367/how-to-set-boundary-while-using-xmlhttprequest-and-formdata
🏁 Script executed:
# Search for the client library in node_modules or package files
fd -name "package.json" | xargs grep -l "tatevikgr\|rest-api-client" 2>/dev/null | head -5
# Search for any client initialization/import to understand its origin
rg -n "@tatevikgr|rest-api-client|apiClient\s*=" --type vue --type ts --type js | head -20
# Look for other FormData + apiClient patterns to see if this is consistent
rg -n "new FormData" -A 10 -B 2 --type vue --type ts --type jsRepository: phpList/web-frontend
Length of output: 350
🌐 Web query:
@tatevikgr/rest-api-client npm documentation FormData headers
💡 Result:
No official npm package named "@tatevikgr/rest-api-client" exists. Extensive searches across npm registry, GitHub, and LinkedIn profiles for "tatevikgr" returned no matching results for this specific package or its documentation. The package may be private, unpublished, or the name misspelled. No documentation on FormData headers was found. General best practices for FormData headers in REST API clients (from similar TypeScript clients like typed-rest-client and ts-rest-client): - Do not manually set Content-Type header when using FormData; let the browser/Node runtime set it automatically with the proper boundary. - Use formData.getHeaders (if using form-data package in Node) to get required headers. - Example with fetch: const formData = new FormData; formData.append('file', file); fetch(url, { method: 'POST', body: formData }); // No Content-Type header - For Node.js with form-data package: const FormData = require('form-data'); const formData = new FormData; formData.append('file', file); const headers = formData.getHeaders; fetch(url, { method: 'POST', headers, body: formData }); Sources confirming this: StackOverflow discussions on fetch with FormData, typed-rest-client GitHub issues on form-data usage.
Citations:
- 1: https://ts-rest.com/client/fetch
- 2: Using form-data with typed-rest-client form-data/form-data#410
- 3: https://stackoverflow.com/questions/43500514/how-to-post-with-multipart-form-data-header-and-formdata-using-fetch
- 4: https://registry.npmjs.org/typed-rest-client
- 5: https://github.com/inforion/ts-api-client-generator
🏁 Script executed:
# Find where apiClient is imported/defined
rg -n "apiClient\s*=|import.*apiClient|from.*apiClient" -A 2 -B 2
# Find the client implementation file by searching imports
rg -n "rest-api-client" -A 2 -B 2
# Search for FormData usage patterns more broadly
rg -n "new FormData" -A 8 -B 2
# Look for apiClient.put usage across the codebase
rg -n "apiClient\.put" -A 3 -B 2Repository: phpList/web-frontend
Length of output: 4804
🏁 Script executed:
# Read api.js to see how apiClient is defined
cat -n assets/vue/api.js | head -50
# Check for other multipart/form-data or FormData usages
rg -n "multipart/form-data|new FormData" -A 5 -B 2Repository: phpList/web-frontend
Length of output: 4435
🏁 Script executed:
# Search for how the client.put method is implemented or used elsewhere
rg -n "\.put\(" -B 2 -A 5
# Look for any indication of how the rest-api-client handles FormData
rg -n "fetch|XMLHttpRequest|axios" assets/vue/
# Check if there's any special handling in the client initialization or usage
rg -n "setSessionId|Client\(" -A 3 -B 1 assets/vue/api.jsRepository: phpList/web-frontend
Length of output: 6571
Remove the manual Content-Type header from the FormData request.
When passing FormData to apiClient.put(), the browser automatically sets Content-Type: multipart/form-data; boundary=... with the correct boundary. Manually setting 'Content-Type': 'multipart/form-data' without the boundary parameter overrides this and causes the server to fail parsing the multipart request.
Proposed fix
- await apiClient.put(`templates/${templateId.value}`, payload, {
- 'Content-Type': 'multipart/form-data'
- })
+ await apiClient.put(`templates/${templateId.value}`, payload)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await apiClient.put(`templates/${templateId.value}`, payload, { | |
| 'Content-Type': 'multipart/form-data' | |
| }) | |
| await apiClient.put(`templates/${templateId.value}`, payload) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/views/TemplateEditView.vue` around lines 223 - 225, The request
manually sets the Content-Type header when sending FormData which breaks
boundary handling; in the TemplateEditView.vue code that calls
apiClient.put(`templates/${templateId.value}`, payload, { 'Content-Type':
'multipart/form-data' }), remove the manual Content-Type header (or the entire
third arg if it only contains that header) so the browser/HTTP client can set
multipart/form-data with the correct boundary automatically.
| return $this->render('@PhpListFrontend/spa.html.twig', [ | ||
| 'page' => 'Campaigns', | ||
| 'api_token' => $request->getSession()->get('auth_token'), | ||
| 'api_base_url' => $this->getParameter('api_base_url'), |
There was a problem hiding this comment.
Set the templates page title correctly.
The templates index currently renders with page => 'Campaigns', so the server-rendered title is wrong before Vue takes over.
Proposed fix
return $this->render('@PhpListFrontend/spa.html.twig', [
- 'page' => 'Campaigns',
+ 'page' => 'Templates',
'api_token' => $request->getSession()->get('auth_token'),
'api_base_url' => $this->getParameter('api_base_url'),
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return $this->render('@PhpListFrontend/spa.html.twig', [ | |
| 'page' => 'Campaigns', | |
| 'api_token' => $request->getSession()->get('auth_token'), | |
| 'api_base_url' => $this->getParameter('api_base_url'), | |
| return $this->render('@PhpListFrontend/spa.html.twig', [ | |
| 'page' => 'Templates', | |
| 'api_token' => $request->getSession()->get('auth_token'), | |
| 'api_base_url' => $this->getParameter('api_base_url'), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Controller/TemplatesController.php` around lines 18 - 21, The
TemplatesController is passing the wrong page title to the server-rendered SPA;
in the render call inside TemplatesController::index (the call to
$this->render('@PhpListFrontend/spa.html.twig', [...])) change the 'page'
parameter from 'Campaigns' to the correct template page name (e.g., 'Templates')
so the initial server-rendered title is accurate before Vue hydrates.
| #[Route('/{templateId}/edit', name: 'edit', methods: ['GET'])] | ||
| public function edit(Request $request, int $templateId): Response |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Symfony PHP attribute Route requirements named argument numeric parameter example
💡 Result:
In Symfony, use the PHP #[Route] attribute to define routes with requirements for named numeric parameters. The requirements option accepts an array mapping parameter names to regex patterns (e.g., \d+ for one or more digits). Example from Symfony documentation and tutorials: php namespace App\Controller; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Routing\Attribute\Route; class BlogController extends AbstractController { #[Route('/blog/{page}', name: 'blog_list', requirements: ['page' => '\d+'])] public function list(int $page = 1): Response { // ... } } This ensures /blog/{page} only matches if page is numeric (e.g., /blog/2 works, /blog/abc does not). Use int $page type hint for additional validation. For Symfony 6.1+, use Requirement::DIGITS constant: requirements: ['page' => Requirement::DIGITS]. Enable attribute routing in config/routes.yaml: controllers: resource: ../src/Controller/ type: attribute
Citations:
- 1: https://symfony.com/doc/current/routing.html
- 2: https://symfony.com/doc/current/routing/slash_in_parameter.html
- 3: https://symfony.com/doc/8.0/routing.html
- 4: https://zetcode.com/symfony/route-attribute
- 5: https://symfony.com/doc/5.x/routing/.html
🏁 Script executed:
cd /root/repo && find . -name "composer.json" -type f -exec cat {} \; | grep -A 5 -B 5 "symfony"Repository: phpList/web-frontend
Length of output: 124
🏁 Script executed:
cd /root/repo && head -30 src/Controller/TemplatesController.phpRepository: phpList/web-frontend
Length of output: 124
🏁 Script executed:
cd /root/repo && rg "requirements:" src/Controller/ -A 1 -B 1 | head -20Repository: phpList/web-frontend
Length of output: 124
🏁 Script executed:
cat composer.json | grep -A 5 -B 5 "symfony"Repository: phpList/web-frontend
Length of output: 1352
🏁 Script executed:
head -30 src/Controller/TemplatesController.phpRepository: phpList/web-frontend
Length of output: 1157
🏁 Script executed:
rg "requirements:" src/Controller/ -A 1 -B 1Repository: phpList/web-frontend
Length of output: 46
Constrain templateId at the route level.
/{templateId}/edit currently accepts any segment while the action requires int $templateId; malformed URLs should 404 instead of reaching the controller.
Proposed fix
- #[Route('/{templateId}/edit', name: 'edit', methods: ['GET'])]
+ #[Route('/{templateId}/edit', name: 'edit', requirements: ['templateId' => '\d+'], methods: ['GET'])]
public function edit(Request $request, int $templateId): ResponseAlternatively, since the project uses Symfony 6.4, you can use the Requirement::DIGITS constant:
- #[Route('/{templateId}/edit', name: 'edit', methods: ['GET'])]
+ #[Route('/{templateId}/edit', name: 'edit', requirements: ['templateId' => Requirement::DIGITS], methods: ['GET'])]
public function edit(Request $request, int $templateId): Response(Don't forget to import Symfony\Component\Routing\Requirement if using the constant approach.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Controller/TemplatesController.php` around lines 25 - 26, The route for
the edit action accepts any path segment but the controller signature requires
an int $templateId, so add a route requirement to constrain templateId to digits
to cause malformed URLs to 404; update the attribute on the edit method (public
function edit(Request $request, int $templateId): Response) to include
requirements={'templateId': '\d+'} or use Requirement::DIGITS and import
Symfony\Component\Routing\Requirement if you prefer the constant form.
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!