Open
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 mailing-list management end-to-end: a new Symfony ListsController with /lists routes that serve SPA HTML or JSON; frontend router routes for Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant ListsController
participant ListDirectory
participant ListClient
participant API
Note over ListDirectory,API: List listing and create/edit flows
User->>Browser: Navigate to /lists
Browser->>ListsController: GET /lists
alt Non-XHR HTML request
ListsController->>Browser: Return spa.html.twig (page, api_token, api_base_url)
Browser->>ListDirectory: Mount component
end
ListDirectory->>ListClient: getLists()
ListClient->>API: HTTP GET /lists
API->>ListClient: Return lists JSON
ListClient->>ListDirectory: Return lists
User->>Browser: Open CreateListModal and submit
Browser->>ListClient: createList(request)
ListClient->>API: HTTP POST /lists
API->>ListClient: Return created list
ListClient->>ListDirectory: created list -> refresh lists
sequenceDiagram
participant User
participant Browser
participant ListsController
participant ListSubscribersView
participant SubscriptionClient
participant SubscribersClient
participant API
Note over ListSubscribersView,API: Add/Copy/Move/Export subscribers flows
User->>Browser: Open /lists/{id}/subscribers
Browser->>ListsController: GET /lists/{id}/subscribers
ListsController->>Browser: Return spa.html.twig (page, api_token, api_base_url)
Browser->>ListSubscribersView: Mount view with listId
ListSubscribersView->>SubscriptionClient: getSubscribersOfList(...)
SubscriptionClient->>API: HTTP GET /lists/{id}/subscribers
API->>SubscriptionClient: Return subscribers page
SubscriptionClient->>ListSubscribersView: Return subscribers
User->>Browser: Click "Export"
ListSubscribersView->>SubscribersClient: exportSubscribers(request)
SubscribersClient->>API: HTTP POST /subscribers/export
API->>SubscribersClient: Return CSV response + headers
SubscribersClient->>Browser: (proxied via controller) Browser navigates to CSV URL / download
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 5
🧹 Nitpick comments (2)
assets/vue/components/lists/ListDirectory.vue (2)
242-248: Keep thedeleteevent contract honest.Line 243 declares a
deleteemit, buthandleDelete()never fires it. If delete is intentionally self-contained, drop that event fromdefineEmits; otherwise emit it after a successful delete so parent listeners are not silently skipped.💡 Possible fix
try { await listClient.deleteList(list.id) + emit('delete', list) await fetchMailingLists() } catch (e) { console.error('Delete failed', e) }Also applies to: 250-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/lists/ListDirectory.vue` around lines 242 - 248, The defineEmits list declares a 'delete' event but handleDelete() never emits it—either remove 'delete' from the defineEmits array to keep the contract honest, or update the delete flow (in the handleDelete function and any other delete handlers around the 250-259 area) to call emit('delete', payload) after a successful delete (e.g., emit('delete', { id: itemId } ) or appropriate payload) so parent listeners receive the event; make this change in the ListDirectory.vue methods referencing handleDelete and any other delete handler functions.
46-90: Consider deriving both action groups from one source.The desktop and mobile layouts duplicate the same action set, and the copy has already drifted (
Add SubscribersvsAdd Subscriber). A smallactionsconfig or child action-row component would keep labels, icons, handlers, and button styling in sync.Also applies to: 123-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/lists/ListDirectory.vue` around lines 46 - 90, The duplicated action buttons should be derived from a single source to avoid drift; refactor ListDirectory.vue to define an actions array (or a small child component) that contains entries with label, icon, css classes, and handler references (e.g., handleDelete, handleAddSubscriber, handleEdit, handleStartCampaign, handleViewMembers) and then render that array in both desktop and mobile layouts (or pass it into an ActionRow component) so labels (fix “Add Subscribers” vs “Add Subscriber”), icons, handlers, and styling remain in sync across 46-90 and 123-162.
🤖 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/lists/CreateListModal.vue`:
- Around line 22-23: The primary CTA in CreateListModal.vue is a type="button"
outside the form so Enter-submits are flaky; update the modal so the primary
footer CTA invokes the form submit normally by making it a real submit button
(type="submit") inside the <form> that uses the submitCreateList handler (or
move the button into the form if currently outside), and mirror the same change
in EditListModal.vue and AddSubscribersModal.vue so their primary CTAs also use
type="submit" and rely on the existing `@submit.prevent` handlers
(submitCreateList / corresponding submit handlers) instead of click-only
buttons.
In `@assets/vue/components/lists/EditListModal.vue`:
- Around line 93-106: The label "for" attributes for the RSS feed and subject
prefix fields don't match their input ids (label for="list-rssFeed" vs input
id="list-rss", and label for="list-subjectPrefix" vs input id="list-prefix"),
breaking focus and accessibility; update either the labels or the input ids so
they match (e.g., change the RSS label to for="list-rss" and the subject prefix
label to for="list-prefix" or rename the inputs to
list-rssFeed/list-subjectPrefix) while keeping the v-model bindings
(editForm.rssFeed and editForm.subjectPrefix) intact.
- Around line 166-173: fillEditForm currently mixes listPosition and
list_position and sets the empty branch to null, but submitEditList expects an
empty string as “unset”; update fillEditForm to consistently read the correct
property (props.list.listPosition or props.list.list_position — pick the actual
prop name used elsewhere) and set editForm.value.listPosition to '' (empty
string) instead of null when no position is present so submitEditList validation
and save behavior work correctly.
In `@assets/vue/components/lists/ListDirectory.vue`:
- Around line 95-99: The component currently treats any empty mailingLists as
"No mailing lists found" and calls response.json() without checking response.ok
or content-type, causing auth redirects or server errors to clear the list and
show the empty state; add refs isLoading and loadError, set isLoading = true
before the fetch in the method that loads lists (where response.json() is
called), check response.ok and response.headers.get('content-type') includes
'application/json' before parsing, set loadError with a useful message when
response.ok is false or parsing fails, clear mailingLists only on successful
parse, and set isLoading = false in finally; update the template to show a
loading indicator when isLoading, an error message when loadError, and only show
the "No mailing lists found" row when !isLoading && !loadError &&
mailingLists.length === 0 (referencing mailingLists, isLoading, loadError, and
the fetch/response.json() usage to locate changes).
In `@src/Controller/ListsController.php`:
- Around line 24-25: The Accept header check in the conditional around
isXmlHttpRequest currently only matches exactly 'application/json' and will miss
composite headers; update the condition in the controller (the if using
$request->isXmlHttpRequest() and $request->headers->get('Accept')) to detect
JSON more robustly by checking whether the Accept header contains
'application/json' (e.g., use strpos or
$request->getAcceptableContentTypes()/in_array logic) before deciding to
render('spa.html.twig'), so requests with composite Accept values like
'application/json, text/plain, */*' are treated as JSON callers.
---
Nitpick comments:
In `@assets/vue/components/lists/ListDirectory.vue`:
- Around line 242-248: The defineEmits list declares a 'delete' event but
handleDelete() never emits it—either remove 'delete' from the defineEmits array
to keep the contract honest, or update the delete flow (in the handleDelete
function and any other delete handlers around the 250-259 area) to call
emit('delete', payload) after a successful delete (e.g., emit('delete', { id:
itemId } ) or appropriate payload) so parent listeners receive the event; make
this change in the ListDirectory.vue methods referencing handleDelete and any
other delete handler functions.
- Around line 46-90: The duplicated action buttons should be derived from a
single source to avoid drift; refactor ListDirectory.vue to define an actions
array (or a small child component) that contains entries with label, icon, css
classes, and handler references (e.g., handleDelete, handleAddSubscriber,
handleEdit, handleStartCampaign, handleViewMembers) and then render that array
in both desktop and mobile layouts (or pass it into an ActionRow component) so
labels (fix “Add Subscribers” vs “Add Subscriber”), icons, handlers, and styling
remain in sync across 46-90 and 123-162.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43205605-7e5a-4eba-986e-72903a4ca75f
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
assets/router/index.jsassets/vue/api.jsassets/vue/components/lists/AddSubscribersModal.vueassets/vue/components/lists/CreateListModal.vueassets/vue/components/lists/EditListModal.vueassets/vue/components/lists/ListDirectory.vueassets/vue/views/ListsView.vuepackage.jsonsrc/Controller/ListsController.php
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/lists/ListSubscribersExportPanel.vue`:
- Around line 101-115: The code is only fetching the first 5 attributes and then
unconditionally overwriting form.value.columns, which prevents exporting later
attributes and clobbers user selections; update the client.get('attributes',
...) call in the onMounted block to retrieve the full attribute set (remove or
increase the limit and/or implement pagination to accumulate all pages) and when
applying dynamicColumns to columnOptions and form.value.columns, merge with
existing selections instead of replacing them (e.g., append new column values to
columnOptions.value and set form.value.columns only if it's empty or by taking
the union of existing form.value.columns and new dynamicColumns) so user choices
are preserved.
- Around line 170-176: allColumnsSelected currently compares
form.value.columns.length to columnOptions.length but columnOptions is a ref, so
columnOptions.length is undefined; update the computed to use
columnOptions.value.length (i.e., compare form.value.columns.length ===
columnOptions.value.length) so the "Select all" checkbox can reach checked
state, and keep someColumnsSelected as-is but relying on
allColumnsSelected.value for correctness.
In `@assets/vue/views/ListSubscribersView.vue`:
- Around line 522-531: Move mutations of cursorHistory until after a successful
fetch: in nextPage() avoid pushing nextCursor.value before awaiting
fetchSubscribers(nextCursor.value); instead call fetchSubscribers(...) first and
only push nextCursor.value if the call succeeds (no exception/false result).
Similarly, in previousPage() avoid popping cursorHistory.value before awaiting
fetchSubscribers(getCurrentCursor()); call fetchSubscribers(...) first and only
pop cursorHistory.value after a successful fetch. Use the existing functions
nextPage, previousPage, fetchSubscribers, cursorHistory, nextCursor and
getCurrentCursor to locate and update the logic.
- Around line 424-427: The Move Selected flow does a copy-then-delete
(createSubscriptions → deleteEmailsFromCurrentList) but doesn’t handle the case
where create succeeds and delete fails, leaving duplicate membership and a
generic error; update the move handler and deleteEmailsFromCurrentList to detect
partial success: after calling createSubscriptions (or the function that creates
in the target list), attempt the delete, and if the delete call fails, either
(a) perform a compensating delete of the just-created subscriptions in the
target list via the same client.delete (use the target list id and the same
emails) to roll back, or (b) surface a clear UI error indicating partial success
and include the list id(s) so the user can resolve duplicates; implement
retry/rollback logic in deleteEmailsFromCurrentList and ensure the same pattern
is applied in the other Move Selected implementation referenced around lines
496-513 (the other move handler).
In `@src/Controller/SubscribersController.php`:
- Around line 113-132: The controller currently buffers the entire export by
casting $upstreamResponse->getBody() to a string and always returns a 200 via
new Response($content); instead, stream the upstream body and preserve its
status and headers: obtain $upstreamResponse from
$this->subscribersClient->exportSubscribers($exportRequest), create a Symfony
StreamedResponse (or equivalent streaming response) that reads and echoes from
$upstreamResponse->getBody() (loop until EOF) without buffering, set the
response status to $upstreamResponse->getStatusCode(), and copy upstream headers
like Content-Type and Content-Disposition (falling back to your existing
defaults) onto the streamed response before returning it from
SubscribersController.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23762644-34b5-4be9-83d6-9c46e7275d9c
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
assets/router/index.jsassets/vue/api.jsassets/vue/components/lists/CreateListModal.vueassets/vue/components/lists/EditListModal.vueassets/vue/components/lists/ListDirectory.vueassets/vue/components/lists/ListSubscribersExportPanel.vueassets/vue/components/subscribers/SubscribersTable.vueassets/vue/views/ListSubscribersView.vuepackage.jsonsrc/Controller/ListsController.phpsrc/Controller/SubscribersController.php
💤 Files with no reviewable changes (1)
- assets/vue/components/subscribers/SubscribersTable.vue
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- assets/router/index.js
- assets/vue/api.js
- src/Controller/ListsController.php
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
assets/vue/components/lists/ListSubscribersExportPanel.vue (2)
120-135:⚠️ Potential issue | 🟠 MajorDon't stop at the first 5 attributes or reset selections after mount.
limit: 5means later custom attributes never become exportable, and the asyncform.value.columns = ...reset can wipe out choices the user makes before the request finishes. Fetch the full attribute set and merge it into the current selection instead of replacing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/lists/ListSubscribersExportPanel.vue` around lines 120 - 135, The mounted hook uses client.get('attributes', { limit: 5 }) and then overwrites columnOptions and form.value.columns, which limits available custom attributes and can clobber user selections; change onMounted to request the full attribute list (remove or increase the limit parameter / request all pages) via client.get('attributes'), build dynamicColumns from all returned items, merge dynamicColumns into the existing columnOptions.value (not replace) and update form.value.columns by merging new column values into the existing form.value.columns selection (preserve prior selections) rather than assigning columnOptions.value wholesale; update references in the onMounted block (client.get, dynamicColumns, columnOptions.value, form.value.columns, capitalizeFirst) accordingly.
188-190:⚠️ Potential issue | 🟡 MinorUse
columnOptions.value.lengthin script code.
columnOptionsis a ref here, socolumnOptions.lengthisundefined. The “Select all” checkbox never reaches the checked state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/lists/ListSubscribersExportPanel.vue` around lines 188 - 190, The computed property allColumnsSelected is comparing form.value.columns.length to columnOptions.length but columnOptions is a ref, so use columnOptions.value.length instead; update the computed (allColumnsSelected) to compare against columnOptions.value.length (and ensure any other uses of columnOptions in that component use .value when in script) so the "Select all" checkbox can correctly become checked.src/Controller/SubscribersController.php (1)
113-132:⚠️ Potential issue | 🟠 MajorStream the upstream export instead of buffering it.
This still materializes the whole CSV in PHP and rebuilds it as a fresh
Response, so large exports can spike memory and upstream 4xx/5xx responses get flattened into a “successful” download. Return a streamed response with the upstream status code and headers instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/SubscribersController.php` around lines 113 - 132, The current code buffers the entire upstream response body into $content and returns a new Response, which can OOM and hides upstream status codes; change this to return a streaming response that proxies the upstream status and headers from $upstreamResponse (use $upstreamResponse->getStatusCode() and getHeaderLine for Content-Type/Content-Disposition with the same fallback values if empty) and stream the upstream body directly to the client (e.g. create a Symfony StreamedResponse and in the callback read from $upstreamResponse->getBody() and echo/stream_copy_to_stream until EOF), ensuring you copy other relevant headers rather than rebuilding the body.
🤖 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/lists/ListSubscribersExportPanel.vue`:
- Around line 222-248: The exportSubscribers function currently proceeds when
form.value.columns is empty, causing the backend to fall back to defaults; add a
guard at the start of exportSubscribers to check if form.value.columns is empty
(e.g., form.value.columns.length === 0) and if so set exportError.value to a
user-facing message like "Please select at least one column to export." and
return early; update any existing date validation branch order if needed so the
columns check runs before building params and before redirect, referencing
exportSubscribers and form.value.columns and exportError.value to locate where
to insert this guard.
In `@assets/vue/components/subscribers/SubscriberDirectory.vue`:
- Line 87: ListSubscribersExportPanel is rendered without the active directory
filter so exports ignore the current table search; pass the active currentFilter
(or the directory criteria object used in this component) into
ListSubscribersExportPanel as a prop and update the panel/export request to
include/merge that criteria into the export payload (so the directory-specific
filter is applied when the export builder runs); specifically, in the place
where ListSubscribersExportPanel is instantiated, forward currentFilter (or the
local directory criteria variable) and ensure the export code inside
ListSubscribersExportPanel consumes that prop and includes it when building the
export request.
---
Duplicate comments:
In `@assets/vue/components/lists/ListSubscribersExportPanel.vue`:
- Around line 120-135: The mounted hook uses client.get('attributes', { limit: 5
}) and then overwrites columnOptions and form.value.columns, which limits
available custom attributes and can clobber user selections; change onMounted to
request the full attribute list (remove or increase the limit parameter /
request all pages) via client.get('attributes'), build dynamicColumns from all
returned items, merge dynamicColumns into the existing columnOptions.value (not
replace) and update form.value.columns by merging new column values into the
existing form.value.columns selection (preserve prior selections) rather than
assigning columnOptions.value wholesale; update references in the onMounted
block (client.get, dynamicColumns, columnOptions.value, form.value.columns,
capitalizeFirst) accordingly.
- Around line 188-190: The computed property allColumnsSelected is comparing
form.value.columns.length to columnOptions.length but columnOptions is a ref, so
use columnOptions.value.length instead; update the computed (allColumnsSelected)
to compare against columnOptions.value.length (and ensure any other uses of
columnOptions in that component use .value when in script) so the "Select all"
checkbox can correctly become checked.
In `@src/Controller/SubscribersController.php`:
- Around line 113-132: The current code buffers the entire upstream response
body into $content and returns a new Response, which can OOM and hides upstream
status codes; change this to return a streaming response that proxies the
upstream status and headers from $upstreamResponse (use
$upstreamResponse->getStatusCode() and getHeaderLine for
Content-Type/Content-Disposition with the same fallback values if empty) and
stream the upstream body directly to the client (e.g. create a Symfony
StreamedResponse and in the callback read from $upstreamResponse->getBody() and
echo/stream_copy_to_stream until EOF), ensuring you copy other relevant headers
rather than rebuilding the body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f26edbd0-0172-474a-ae0b-4eb2694e5dc7
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
assets/vue/api.jsassets/vue/components/lists/ListSubscribersExportPanel.vueassets/vue/components/subscribers/SubscriberDirectory.vueassets/vue/components/subscribers/SubscribersTable.vueassets/vue/views/ListSubscribersView.vuepackage.jsonsrc/Controller/SubscribersController.php
💤 Files with no reviewable changes (1)
- assets/vue/components/subscribers/SubscribersTable.vue
✅ Files skipped from review due to trivial changes (2)
- package.json
- assets/vue/views/ListSubscribersView.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- assets/vue/api.js
Comment on lines
+222
to
+248
| const exportSubscribers = () => { | ||
| if (!usesAnyDate.value && form.value.dateFrom && form.value.dateTo && form.value.dateFrom > form.value.dateTo) { | ||
| exportError.value = 'Date From cannot be after Date To.' | ||
| return | ||
| } | ||
|
|
||
| const params = new URLSearchParams() | ||
| if (props.listId) { | ||
| params.set('list_id', String(props.listId)) | ||
| } | ||
| params.set('date_type', form.value.dateType) | ||
|
|
||
| if (!usesAnyDate.value) { | ||
| if (form.value.dateFrom) { | ||
| params.set('date_from', form.value.dateFrom) | ||
| } | ||
|
|
||
| if (form.value.dateTo) { | ||
| params.set('date_to', form.value.dateTo) | ||
| } | ||
| } | ||
|
|
||
| form.value.columns.forEach((column) => { | ||
| params.append('columns[]', column) | ||
| }) | ||
|
|
||
| window.location.href = `/subscribers/export?${params.toString()}` |
There was a problem hiding this comment.
Require at least one selected column before exporting.
When form.value.columns is empty, this method sends no columns[] values at all, and src/Controller/SubscribersController.php falls back to the default column list. A user who deselects everything still gets a non-empty export back.
Quick guard
const exportSubscribers = () => {
+ if (form.value.columns.length === 0) {
+ exportError.value = 'Select at least one column.'
+ return
+ }
+
if (!usesAnyDate.value && form.value.dateFrom && form.value.dateTo && form.value.dateFrom > form.value.dateTo) {
exportError.value = 'Date From cannot be after Date To.'
return
}📝 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.
Suggested change
| const exportSubscribers = () => { | |
| if (!usesAnyDate.value && form.value.dateFrom && form.value.dateTo && form.value.dateFrom > form.value.dateTo) { | |
| exportError.value = 'Date From cannot be after Date To.' | |
| return | |
| } | |
| const params = new URLSearchParams() | |
| if (props.listId) { | |
| params.set('list_id', String(props.listId)) | |
| } | |
| params.set('date_type', form.value.dateType) | |
| if (!usesAnyDate.value) { | |
| if (form.value.dateFrom) { | |
| params.set('date_from', form.value.dateFrom) | |
| } | |
| if (form.value.dateTo) { | |
| params.set('date_to', form.value.dateTo) | |
| } | |
| } | |
| form.value.columns.forEach((column) => { | |
| params.append('columns[]', column) | |
| }) | |
| window.location.href = `/subscribers/export?${params.toString()}` | |
| const exportSubscribers = () => { | |
| if (form.value.columns.length === 0) { | |
| exportError.value = 'Select at least one column.' | |
| return | |
| } | |
| if (!usesAnyDate.value && form.value.dateFrom && form.value.dateTo && form.value.dateFrom > form.value.dateTo) { | |
| exportError.value = 'Date From cannot be after Date To.' | |
| return | |
| } | |
| const params = new URLSearchParams() | |
| if (props.listId) { | |
| params.set('list_id', String(props.listId)) | |
| } | |
| params.set('date_type', form.value.dateType) | |
| if (!usesAnyDate.value) { | |
| if (form.value.dateFrom) { | |
| params.set('date_from', form.value.dateFrom) | |
| } | |
| if (form.value.dateTo) { | |
| params.set('date_to', form.value.dateTo) | |
| } | |
| } | |
| form.value.columns.forEach((column) => { | |
| params.append('columns[]', column) | |
| }) | |
| window.location.href = `/subscribers/export?${params.toString()}` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/lists/ListSubscribersExportPanel.vue` around lines 222
- 248, The exportSubscribers function currently proceeds when form.value.columns
is empty, causing the backend to fall back to defaults; add a guard at the start
of exportSubscribers to check if form.value.columns is empty (e.g.,
form.value.columns.length === 0) and if so set exportError.value to a
user-facing message like "Please select at least one column to export." and
return early; update any existing date validation branch order if needed so the
columns check runs before building params and before redirect, referencing
exportSubscribers and form.value.columns and exportError.value to locate where
to insert this guard.
| </div> | ||
| </div> | ||
|
|
||
| <ListSubscribersExportPanel/> |
There was a problem hiding this comment.
Pass the directory criteria into this panel.
ListSubscribersExportPanel bypasses the existing directory-specific export builder in this file and only serializes list_id, date fields, and columns[]. The active currentFilter / search state never reaches the export request, so exporting from a filtered table can pull a broader subscriber set than the one on screen.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/subscribers/SubscriberDirectory.vue` at line 87,
ListSubscribersExportPanel is rendered without the active directory filter so
exports ignore the current table search; pass the active currentFilter (or the
directory criteria object used in this component) into
ListSubscribersExportPanel as a prop and update the panel/export request to
include/merge that criteria into the export payload (so the directory-specific
filter is applied when the export builder runs); specifically, in the place
where ListSubscribersExportPanel is instantiated, forward currentFilter (or the
local directory criteria variable) and ensure the export code inside
ListSubscribersExportPanel consumes that prop and includes it when building the
export request.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/Controller/ListsController.php (1)
38-46: Consider noting the unused$listIdparameter.The
$listIdparam is captured from the route but not used server-side. This is fine since the SPA reads it client-side, but you could add a brief comment or use#[SuppressWarnings("UnusedParameter")]to clarify the intent and avoid static analysis warnings.✨ Optional: Add annotation or comment
#[Route('/{listId}/subscribers', name: 'list_subscribers', methods: ['GET'])] + /** `@SuppressWarnings`(PHPMD.UnusedFormalParameter) listId is used client-side by the SPA */ public function view(Request $request, int $listId): JsonResponse|Response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/ListsController.php` around lines 38 - 46, The route handler view(Request $request, int $listId) captures $listId from the route but never uses it, which can trigger static-analysis warnings; update the method to either add a concise inline comment explaining the ID is used by the SPA (e.g. "// listId consumed client-side by SPA") or annotate the method to suppress unused-parameter warnings (e.g. add an appropriate suppression attribute or docblock such as a `@noinspection/`#[SuppressWarnings("UnusedParameter")] on the view method) so tools and readers understand this is intentional; modify the view function declaration in ListsController::view accordingly.
🤖 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/lists/CreateListModal.vue`:
- Around line 150-151: The parsing of listPosition in CreateListModal.vue should
trim whitespace before deciding emptiness and coercion: replace the existing
parsedPosition expression that uses createForm.value.listPosition directly with
a trimmed check and trimmed Number conversion (e.g. use const raw =
createForm.value.listPosition?.trim() ?? ''; const parsedPosition = raw === '' ?
null : Number(raw)) so whitespace-only input doesn't become 0; update references
to parsedPosition accordingly.
- Around line 17-19: The close button in CreateListModal.vue is icon-only and
lacks an accessible name; update the button that calls the close method (the
<button ... `@click`="close"> wrapping BaseIcon) to provide an explicit label for
assistive tech by adding either an aria-label="Close" (or aria-label bound to a
localized string) or include a visually hidden text node (e.g., a span with the
app’s "sr-only" class containing "Close") alongside the BaseIcon so screen
readers can announce the button.
In `@assets/vue/components/lists/EditListModal.vue`:
- Around line 17-19: The close button in EditListModal.vue (the button with
`@click`="close" containing <BaseIcon name="close" />) lacks an accessible name;
add an explicit label by adding an aria-label (e.g., aria-label="Close") or
include visually hidden text (e.g., a span with sr-only content "Close") inside
the button so screen readers announce the action, ensuring the label accurately
describes the button's behavior.
---
Nitpick comments:
In `@src/Controller/ListsController.php`:
- Around line 38-46: The route handler view(Request $request, int $listId)
captures $listId from the route but never uses it, which can trigger
static-analysis warnings; update the method to either add a concise inline
comment explaining the ID is used by the SPA (e.g. "// listId consumed
client-side by SPA") or annotate the method to suppress unused-parameter
warnings (e.g. add an appropriate suppression attribute or docblock such as a
`@noinspection/`#[SuppressWarnings("UnusedParameter")] on the view method) so
tools and readers understand this is intentional; modify the view function
declaration in ListsController::view accordingly.
🪄 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: 42b51355-aa9c-46e7-975e-917810e2a081
📒 Files selected for processing (8)
assets/vue/components/lists/AddSubscribersModal.vueassets/vue/components/lists/CreateListModal.vueassets/vue/components/lists/EditListModal.vueassets/vue/components/lists/ListDirectory.vueassets/vue/components/lists/ListSubscribersExportPanel.vueassets/vue/views/ListSubscribersView.vuesrc/Controller/ListsController.phpsrc/Controller/SubscribersController.php
🚧 Files skipped from review as they are similar to previous changes (1)
- assets/vue/components/lists/ListDirectory.vue
Comment on lines
+17
to
+19
| <button type="button" class="text-slate-400 hover:text-slate-500" @click="close"> | ||
| <BaseIcon name="close" class="w-5 h-5" /> | ||
| </button> |
There was a problem hiding this comment.
Add an accessible name to the icon-only close button.
Like the edit modal, this dismiss button should expose an explicit label for assistive tech.
💡 Suggested fix
- <button type="button" class="text-slate-400 hover:text-slate-500" `@click`="close">
+ <button
+ type="button"
+ class="text-slate-400 hover:text-slate-500"
+ aria-label="Close create list modal"
+ `@click`="close"
+ >
<BaseIcon name="close" class="w-5 h-5" />
</button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/lists/CreateListModal.vue` around lines 17 - 19, The
close button in CreateListModal.vue is icon-only and lacks an accessible name;
update the button that calls the close method (the <button ... `@click`="close">
wrapping BaseIcon) to provide an explicit label for assistive tech by adding
either an aria-label="Close" (or aria-label bound to a localized string) or
include a visually hidden text node (e.g., a span with the app’s "sr-only" class
containing "Close") alongside the BaseIcon so screen readers can announce the
button.
Comment on lines
+150
to
+151
| const parsedPosition = createForm.value.listPosition === '' ? null : Number(createForm.value.listPosition) | ||
|
|
There was a problem hiding this comment.
Trim listPosition before numeric coercion.
Number(...) without trimming can turn whitespace-like values into 0. Align this with Edit modal parsing to avoid accidental acceptance.
💡 Suggested fix
- const parsedPosition = createForm.value.listPosition === '' ? null : Number(createForm.value.listPosition)
+ const rawPosition = String(createForm.value.listPosition).trim()
+ const parsedPosition = rawPosition === '' ? null : Number(rawPosition)📝 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.
Suggested change
| const parsedPosition = createForm.value.listPosition === '' ? null : Number(createForm.value.listPosition) | |
| const rawPosition = String(createForm.value.listPosition).trim() | |
| const parsedPosition = rawPosition === '' ? null : Number(rawPosition) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/lists/CreateListModal.vue` around lines 150 - 151, The
parsing of listPosition in CreateListModal.vue should trim whitespace before
deciding emptiness and coercion: replace the existing parsedPosition expression
that uses createForm.value.listPosition directly with a trimmed check and
trimmed Number conversion (e.g. use const raw =
createForm.value.listPosition?.trim() ?? ''; const parsedPosition = raw === '' ?
null : Number(raw)) so whitespace-only input doesn't become 0; update references
to parsedPosition accordingly.
Comment on lines
+17
to
+19
| <button type="button" class="text-slate-400 hover:text-slate-500" @click="close"> | ||
| <BaseIcon name="close" class="w-5 h-5" /> | ||
| </button> |
There was a problem hiding this comment.
Add an accessible name to the icon-only close button.
This button needs an explicit accessible label so screen-reader users get a clear action name.
💡 Suggested fix
- <button type="button" class="text-slate-400 hover:text-slate-500" `@click`="close">
+ <button
+ type="button"
+ class="text-slate-400 hover:text-slate-500"
+ aria-label="Close edit list modal"
+ `@click`="close"
+ >
<BaseIcon name="close" class="w-5 h-5" />
</button>📝 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.
Suggested change
| <button type="button" class="text-slate-400 hover:text-slate-500" @click="close"> | |
| <BaseIcon name="close" class="w-5 h-5" /> | |
| </button> | |
| <button | |
| type="button" | |
| class="text-slate-400 hover:text-slate-500" | |
| aria-label="Close edit list modal" | |
| `@click`="close" | |
| > | |
| <BaseIcon name="close" class="w-5 h-5" /> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/lists/EditListModal.vue` around lines 17 - 19, The
close button in EditListModal.vue (the button with `@click`="close" containing
<BaseIcon name="close" />) lacks an accessible name; add an explicit label by
adding an aria-label (e.g., aria-label="Close") or include visually hidden text
(e.g., a span with sr-only content "Close") inside the button so screen readers
announce the action, ensuring the label accurately describes the button's
behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Controller/SubscribersController.php (1)
34-36: Broaden JSON negotiation a bit.This still misses JSON media types like
application/problem+jsonorapplication/vnd.api+json, so those clients would getspa.html.twigback instead of JSON.💡 Small hardening tweak
- $wantsJson = $request->isXmlHttpRequest() || str_contains($accept, 'application/json'); + $wantsJson = $request->isXmlHttpRequest() + || str_contains($accept, 'application/json') + || str_contains($accept, '+json');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/SubscribersController.php` around lines 34 - 36, The JSON negotiation is too narrow: update the $wantsJson check (where $accept and $wantsJson are set, and the branch that returns spa.html.twig) to treat any media type that contains "json" (including +json vendor/problem types) as JSON. Replace the current str_contains($accept, 'application/json') check with a broader check such as checking if the Accept header contains 'json' (e.g. str_contains($accept, 'json') or a preg_match that looks for "json" in media types), while still keeping the isXmlHttpRequest() check, so requests with application/problem+json or application/vnd.api+json will be handled as JSON.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Controller/SubscribersController.php`:
- Around line 91-103: The code is synthetizing CSV download headers (via
$contentType and $contentDisposition fallbacks from
$upstreamResponse->getHeaderLine) even for non-2xx upstream responses; change
the logic to only apply the 'text/csv; charset=UTF-8' and default attachment
filename when $upstreamResponse->getStatusCode() is in the 200–299 range (i.e.
successful export). Update both places that set $contentType and
$contentDisposition (and the other occurrence at lines 116-117) so that for
non-success responses you do not override headers and instead propagate the
upstream status/headers to the client.
---
Nitpick comments:
In `@src/Controller/SubscribersController.php`:
- Around line 34-36: The JSON negotiation is too narrow: update the $wantsJson
check (where $accept and $wantsJson are set, and the branch that returns
spa.html.twig) to treat any media type that contains "json" (including +json
vendor/problem types) as JSON. Replace the current str_contains($accept,
'application/json') check with a broader check such as checking if the Accept
header contains 'json' (e.g. str_contains($accept, 'json') or a preg_match that
looks for "json" in media types), while still keeping the isXmlHttpRequest()
check, so requests with application/problem+json or application/vnd.api+json
will be handled as JSON.
🪄 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: fdff21bc-309d-43f7-90bc-4e4078de4873
📒 Files selected for processing (1)
src/Controller/SubscribersController.php
Comment on lines
+91
to
+103
| $contentType = $upstreamResponse->getHeaderLine('Content-Type'); | ||
| if ($contentType === '') { | ||
| $contentType = 'text/csv; charset=UTF-8'; | ||
| } | ||
|
|
||
| $contentDisposition = $upstreamResponse->getHeaderLine('Content-Disposition'); | ||
| if ($contentDisposition === '') { | ||
| $contentDisposition = sprintf( | ||
| 'attachment; filename="subscribers_export_%s.csv"', | ||
| date('Y-m-d_H-i-s') | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Don't synthesize CSV download headers for upstream failures.
Because assets/vue/components/lists/ListSubscribersExportPanel.vue:243-270 triggers this via window.location.href, a non-2xx upstream response without its own headers will currently be forced into a downloaded .csv. That hides the real error path from the user. Only apply the CSV fallbacks for successful exports.
🩹 Suggested fix
- $contentType = $upstreamResponse->getHeaderLine('Content-Type');
- if ($contentType === '') {
- $contentType = 'text/csv; charset=UTF-8';
- }
-
- $contentDisposition = $upstreamResponse->getHeaderLine('Content-Disposition');
- if ($contentDisposition === '') {
+ $statusCode = $upstreamResponse->getStatusCode();
+ $contentType = $upstreamResponse->getHeaderLine('Content-Type');
+ $contentDisposition = $upstreamResponse->getHeaderLine('Content-Disposition');
+
+ if ($statusCode >= 200 && $statusCode < 300 && $contentType === '') {
+ $contentType = 'text/csv; charset=UTF-8';
+ }
+
+ if ($statusCode >= 200 && $statusCode < 300 && $contentDisposition === '') {
$contentDisposition = sprintf(
'attachment; filename="subscribers_export_%s.csv"',
date('Y-m-d_H-i-s')
);
}
@@
- $upstreamResponse->getStatusCode()
+ $statusCode
);
- $response->headers->set('Content-Type', $contentType);
- $response->headers->set('Content-Disposition', $contentDisposition);
+ if ($contentType !== '') {
+ $response->headers->set('Content-Type', $contentType);
+ }
+ if ($contentDisposition !== '') {
+ $response->headers->set('Content-Disposition', $contentDisposition);
+ }Also applies to: 116-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Controller/SubscribersController.php` around lines 91 - 103, The code is
synthetizing CSV download headers (via $contentType and $contentDisposition
fallbacks from $upstreamResponse->getHeaderLine) even for non-2xx upstream
responses; change the logic to only apply the 'text/csv; charset=UTF-8' and
default attachment filename when $upstreamResponse->getStatusCode() is in the
200–299 range (i.e. successful export). Update both places that set $contentType
and $contentDisposition (and the other occurrence at lines 116-117) so that for
non-success responses you do not override headers and instead propagate the
upstream status/headers to the client.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Chores
Removed
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!