Conversation
📝 WalkthroughWalkthroughAdds a new Campaigns feature: server routes and controller for /campaigns and /campaigns/{id}/edit that render the SPA with API config; frontend router entries for Campaigns and Campaign Edit views; multiple new Vue components (CampaignDirectory, ViewCampaignModal, CampaignsView, CampaignEditView, CkEditorField) implementing listing, modal view, and edit flows with pagination, filters, actions, and CKEditor integration; new API client exports for campaigns, list messages, statistics, and templates; a DI compiler pass to auto-register REST endpoint classes and package dependency updates (rest-api-client v2, CKEditor packages). Sequence DiagramsequenceDiagram
actor User
participant Browser
participant Router
participant CampaignDirectory
participant CampaignAPI
participant StatsAPI
participant ListAPI
User->>Browser: Navigate to /campaigns
Browser->>Router: route -> CampaignsView
Router->>CampaignDirectory: mount
CampaignDirectory->>CampaignAPI: fetch campaigns (cursor paginated)
CampaignAPI-->>CampaignDirectory: campaign items
CampaignDirectory->>StatsAPI: fetch statistics (paginated)
StatsAPI-->>CampaignDirectory: statistics map
CampaignDirectory->>Browser: render list (filters/pagination)
User->>Browser: trigger campaign action (view/edit/delete/other)
alt View
Browser->>CampaignAPI: fetch campaign details
CampaignAPI-->>Browser: details
Browser->>CampaignDirectory: open ViewCampaignModal
else Mutate (delete/suspend/requeue/copy/resend)
Browser->>CampaignAPI: call action endpoint
CampaignAPI-->>Browser: result
Browser->>ListAPI: fetch associated lists (if required)
ListAPI-->>Browser: lists
Browser->>CampaignDirectory: refresh campaigns
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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: 3
🧹 Nitpick comments (4)
assets/vue/components/campaigns/CampaignDirectory.vue (2)
521-540: Inconsistent API client usage.
handleRequeueusesapiClient.post()directly while other handlers (suspend, delete, copy) usecampaignClient. IfcampaignClienthas asendCampaign()or similar method, consider using it for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/campaigns/CampaignDirectory.vue` around lines 521 - 540, handleRequeue currently calls apiClient.post(`campaigns/${campaignId}/send`) directly which is inconsistent with other handlers that use campaignClient; replace the direct call with the campaignClient wrapper method (e.g., campaignClient.sendCampaign(campaignId) or campaignClient.resend(campaignId)) so behavior is centralized, or if that method doesn't exist add sendCampaign(campaignId) to campaignClient that calls apiClient.post('campaigns/{id}/send') and use it inside handleRequeue while keeping the existing loading/feedback/error handling (including isAuthenticationError, setActionFeedback, setActionLoading and fetchCampaigns).
747-769: Consider server-side pagination for scale.Currently all campaigns are fetched upfront (up to 10k with the guard), then filtered/paginated client-side. This works but may become slow with large campaign counts. Server-side filtering and pagination would scale better.
Also applies to: 869-890
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/campaigns/CampaignDirectory.vue` around lines 747 - 769, fetchAllCampaigns currently pulls all pages client-side (loop in fetchAllCampaigns) and then client-side filters/paginates, which won’t scale; change the UI to request server-side pagination and filtering by updating the component to call campaignClient.getCampaigns with page, pageSize and filter parameters and only fetch the single page needed (replace the while loop in fetchAllCampaigns with a single call that accepts cursor/page and pageSize), update any consumer code that expects the full list (also the similar logic around the other block referenced) to use server pagination metadata (response.pagination.hasMore/nextCursor) to drive next-page requests, and ensure getCampaigns usage includes filter args (search, status, sort) so filtering happens server-side rather than fetching everything then filtering locally (focus changes in fetchAllCampaigns and the other function using the same pattern).src/DependencyInjection/Compiler/RegisterEndpointsPass.php (1)
55-59: RedundantsetAutowired(true)call.
$container->autowire()already creates an autowired definition. The subsequent->setAutowired(true)is redundant.♻️ Simplified version
$container ->autowire($className, $className) - ->setAutowired(true) ->setAutoconfigured(true) ->setPublic(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DependencyInjection/Compiler/RegisterEndpointsPass.php` around lines 55 - 59, Remove the redundant setAutowired(true) call after $container->autowire() in the RegisterEndpointsPass code: the autowire() call already marks the definition as autowired, so delete the ->setAutowired(true) in the method chain that builds the definition for $className (the chain starting with $container->autowire($className, $className) and continuing with setAutoconfigured and setPublic) so the chain becomes autowire(...)->setAutoconfigured(...)->setPublic(...).assets/vue/components/campaigns/ViewCampaignModal.vue (1)
2-17: Add dialog semantics + keyboard close for accessibility.The modal is visually fine, but it’s missing
role="dialog",aria-modal, and Esc-to-close behavior. Adding these will make it much friendlier for keyboard/screen-reader users.♿ Suggested improvement
<div v-if="isViewModalOpen" class="fixed inset-0 z-50 flex items-center justify-center bg-slate-900/50 p-4" + role="dialog" + aria-modal="true" + aria-labelledby="campaign-modal-title" + `@keydown.esc`="emit('close')" `@click.self`="emit('close')" > <div class="w-full max-w-2xl rounded-xl border border-slate-200 bg-white shadow-xl"> <div class="flex items-center justify-between border-b border-slate-200 px-5 py-4"> - <h3 class="text-lg font-semibold text-slate-900">Campaign details: {{ campaign?.id || '-' }}</h3> + <h3 id="campaign-modal-title" class="text-lg font-semibold text-slate-900">Campaign details: {{ campaign?.id || '-' }}</h3>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/campaigns/ViewCampaignModal.vue` around lines 2 - 17, The modal lacks dialog semantics and keyboard handling: add role="dialog" aria-modal="true" and a tabindex (e.g., tabindex="0") on the root modal container and give the title <h3> an id (e.g., modalTitle) and set aria-labelledby="modalTitle"; wire Esc-to-close by adding a keydown handler that calls emit('close') (e.g., `@keydown.esc` on the modal container) and ensure the container receives focus when opened by adding a ref (e.g., modalRef) and a watcher on isViewModalOpen that focuses modalRef when true; keep using the existing emit('close') and campaign?.id in the title.
🤖 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/campaigns/CampaignDirectory.vue`:
- Around line 251-259: The mobile button's visibility check is using the wrong
status string; update the conditional in CampaignDirectory.vue so the mobile
"Copy to draft" button uses campaign.statusKey === 'sent' (same condition as
desktop) instead of 'set'; locate the template block with the button that
references isActionLoading(campaign.id) and
`@click`="handleCopyToDraft(campaign.id)" and change the status literal to 'sent'.
- Around line 352-356: The parsePageQuery function is incorrectly passing
pageSize as the radix to Number.parseInt, causing numbers >= pageSize to parse
incorrectly; update parsePageQuery (and the Number.parseInt call that currently
uses pageSize) to use radix 10 instead (e.g., pass 10 or otherwise parse as
base-10) so page numbers like "6" parse correctly; keep the rest of the logic
(queryValue handling, NaN and <1 fallback) unchanged.
In `@assets/vue/components/campaigns/ViewCampaignModal.vue`:
- Around line 114-120: The user-facing message currently mutates the timestamp
by flooring minutes (variables minutes, flooredMinutes, final) and then uses
that mutated value in the returned string, which can show an earlier "until"
than the real repeatUntil; instead, stop modifying the date used for
display—remove the final.setMinutes(...) mutation and return the message using
the original end (or repeatUntil) value (e.g., end.toLocaleString()) so the
modal shows the actual cutoff; if you still need a floored time for internal
logic, keep that calculation in a separate variable and do not reuse it in the
displayed string.
---
Nitpick comments:
In `@assets/vue/components/campaigns/CampaignDirectory.vue`:
- Around line 521-540: handleRequeue currently calls
apiClient.post(`campaigns/${campaignId}/send`) directly which is inconsistent
with other handlers that use campaignClient; replace the direct call with the
campaignClient wrapper method (e.g., campaignClient.sendCampaign(campaignId) or
campaignClient.resend(campaignId)) so behavior is centralized, or if that method
doesn't exist add sendCampaign(campaignId) to campaignClient that calls
apiClient.post('campaigns/{id}/send') and use it inside handleRequeue while
keeping the existing loading/feedback/error handling (including
isAuthenticationError, setActionFeedback, setActionLoading and fetchCampaigns).
- Around line 747-769: fetchAllCampaigns currently pulls all pages client-side
(loop in fetchAllCampaigns) and then client-side filters/paginates, which won’t
scale; change the UI to request server-side pagination and filtering by updating
the component to call campaignClient.getCampaigns with page, pageSize and filter
parameters and only fetch the single page needed (replace the while loop in
fetchAllCampaigns with a single call that accepts cursor/page and pageSize),
update any consumer code that expects the full list (also the similar logic
around the other block referenced) to use server pagination metadata
(response.pagination.hasMore/nextCursor) to drive next-page requests, and ensure
getCampaigns usage includes filter args (search, status, sort) so filtering
happens server-side rather than fetching everything then filtering locally
(focus changes in fetchAllCampaigns and the other function using the same
pattern).
In `@assets/vue/components/campaigns/ViewCampaignModal.vue`:
- Around line 2-17: The modal lacks dialog semantics and keyboard handling: add
role="dialog" aria-modal="true" and a tabindex (e.g., tabindex="0") on the root
modal container and give the title <h3> an id (e.g., modalTitle) and set
aria-labelledby="modalTitle"; wire Esc-to-close by adding a keydown handler that
calls emit('close') (e.g., `@keydown.esc` on the modal container) and ensure the
container receives focus when opened by adding a ref (e.g., modalRef) and a
watcher on isViewModalOpen that focuses modalRef when true; keep using the
existing emit('close') and campaign?.id in the title.
In `@src/DependencyInjection/Compiler/RegisterEndpointsPass.php`:
- Around line 55-59: Remove the redundant setAutowired(true) call after
$container->autowire() in the RegisterEndpointsPass code: the autowire() call
already marks the definition as autowired, so delete the ->setAutowired(true) in
the method chain that builds the definition for $className (the chain starting
with $container->autowire($className, $className) and continuing with
setAutoconfigured and setPublic) so the chain becomes
autowire(...)->setAutoconfigured(...)->setPublic(...).
🪄 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: 4a0aab80-6949-4742-8299-d20a5b18d442
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
assets/router/index.jsassets/vue/api.jsassets/vue/components/campaigns/CampaignDirectory.vueassets/vue/components/campaigns/ViewCampaignModal.vueassets/vue/views/CampaignsView.vueconfig/services.ymlpackage.jsonsrc/Controller/CampaignsController.phpsrc/DependencyInjection/Compiler/RegisterEndpointsPass.phpsrc/PhpListFrontendBundle.php
💤 Files with no reviewable changes (1)
- config/services.yml
| <button | ||
| v-if="campaign.statusKey === 'set'" | ||
| type="button" | ||
| class="px-2.5 py-1 text-xs font-medium text-slate-700 bg-slate-100 border border-slate-200 rounded-md hover:bg-slate-200 disabled:opacity-50" | ||
| :disabled="isActionLoading(campaign.id)" | ||
| @click="handleCopyToDraft(campaign.id)" | ||
| > | ||
| Copy to draft | ||
| </button> |
There was a problem hiding this comment.
Typo: Mobile "Copy to draft" checks 'set' instead of 'sent'.
The desktop version (line 118) correctly checks campaign.statusKey === 'sent', but the mobile version here checks 'set'. This will prevent the button from appearing on mobile.
🐛 Fix the status check
<button
- v-if="campaign.statusKey === 'set'"
+ v-if="campaign.statusKey === 'sent'"
type="button"
class="px-2.5 py-1 text-xs font-medium text-slate-700 bg-slate-100 border border-slate-200 rounded-md hover:bg-slate-200 disabled:opacity-50"📝 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.
| <button | |
| v-if="campaign.statusKey === 'set'" | |
| type="button" | |
| class="px-2.5 py-1 text-xs font-medium text-slate-700 bg-slate-100 border border-slate-200 rounded-md hover:bg-slate-200 disabled:opacity-50" | |
| :disabled="isActionLoading(campaign.id)" | |
| @click="handleCopyToDraft(campaign.id)" | |
| > | |
| Copy to draft | |
| </button> | |
| <button | |
| v-if="campaign.statusKey === 'sent'" | |
| type="button" | |
| class="px-2.5 py-1 text-xs font-medium text-slate-700 bg-slate-100 border border-slate-200 rounded-md hover:bg-slate-200 disabled:opacity-50" | |
| :disabled="isActionLoading(campaign.id)" | |
| `@click`="handleCopyToDraft(campaign.id)" | |
| > | |
| Copy to draft | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/campaigns/CampaignDirectory.vue` around lines 251 -
259, The mobile button's visibility check is using the wrong status string;
update the conditional in CampaignDirectory.vue so the mobile "Copy to draft"
button uses campaign.statusKey === 'sent' (same condition as desktop) instead of
'set'; locate the template block with the button that references
isActionLoading(campaign.id) and `@click`="handleCopyToDraft(campaign.id)" and
change the status literal to 'sent'.
| const parsePageQuery = (pageQuery) => { | ||
| const queryValue = Array.isArray(pageQuery) ? pageQuery[0] : pageQuery | ||
| const page = Number.parseInt(String(queryValue ?? ''), pageSize) | ||
| return Number.isNaN(page) || page < 1 ? 1 : page | ||
| } |
There was a problem hiding this comment.
Bug: parseInt uses pageSize (5) as radix instead of 10.
Number.parseInt(String(queryValue ?? ''), pageSize) parses the string in base-5. A page number like 6 would fail to parse correctly (returns NaN in base-5). Use radix 10.
🐛 Fix the radix
const parsePageQuery = (pageQuery) => {
const queryValue = Array.isArray(pageQuery) ? pageQuery[0] : pageQuery
- const page = Number.parseInt(String(queryValue ?? ''), pageSize)
+ const page = Number.parseInt(String(queryValue ?? ''), 10)
return Number.isNaN(page) || page < 1 ? 1 : page
}📝 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.
| const parsePageQuery = (pageQuery) => { | |
| const queryValue = Array.isArray(pageQuery) ? pageQuery[0] : pageQuery | |
| const page = Number.parseInt(String(queryValue ?? ''), pageSize) | |
| return Number.isNaN(page) || page < 1 ? 1 : page | |
| } | |
| const parsePageQuery = (pageQuery) => { | |
| const queryValue = Array.isArray(pageQuery) ? pageQuery[0] : pageQuery | |
| const page = Number.parseInt(String(queryValue ?? ''), 10) | |
| return Number.isNaN(page) || page < 1 ? 1 : page | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/campaigns/CampaignDirectory.vue` around lines 352 -
356, The parsePageQuery function is incorrectly passing pageSize as the radix to
Number.parseInt, causing numbers >= pageSize to parse incorrectly; update
parsePageQuery (and the Number.parseInt call that currently uses pageSize) to
use radix 10 instead (e.g., pass 10 or otherwise parse as base-10) so page
numbers like "6" parse correctly; keep the rest of the logic (queryValue
handling, NaN and <1 fallback) unchanged.
TatevikGr
force-pushed
the
feat/campaigns
branch
from
2b3120d to
e5306f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
assets/vue/components/campaigns/CampaignDirectory.vue (2)
251-259:⚠️ Potential issue | 🔴 CriticalMobile "Copy to draft" is still checking
'set'.Desktop uses
'sent', but this branch still uses'set', so the action disappears on smaller screens.💡 Minimal fix
- <button - v-if="campaign.statusKey === 'set'" + <button + v-if="campaign.statusKey === 'sent'"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/campaigns/CampaignDirectory.vue` around lines 251 - 259, The mobile template's "Copy to draft" button condition is using campaign.statusKey === 'set' while desktop expects 'sent', causing the button to vanish on small screens; update the v-if to check for 'sent' instead of 'set' in the CampaignDirectory.vue mobile branch (the button that calls handleCopyToDraft(campaign.id) and uses :disabled="isActionLoading(campaign.id)"), ensuring the conditional aligns with the desktop logic so the action appears consistently.
354-357:⚠️ Potential issue | 🔴 Critical
parseIntis still usingpageSizeas the radix.With
pageSize = 5,?page=5and?page=6both fall apart and pagination snaps back to page 1.💡 Minimal fix
const parsePageQuery = (pageQuery) => { const queryValue = Array.isArray(pageQuery) ? pageQuery[0] : pageQuery - const page = Number.parseInt(String(queryValue ?? ''), pageSize) + const page = Number.parseInt(String(queryValue ?? ''), 10) return Number.isNaN(page) || page < 1 ? 1 : page }#!/bin/bash node - <<'NODE' for (const value of ['4', '5', '6']) { console.log(`${value} ->`, Number.parseInt(value, 5)) } NODE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/campaigns/CampaignDirectory.vue` around lines 354 - 357, The parsePageQuery function is incorrectly passing pageSize as the radix to Number.parseInt, causing wrong parsing (e.g., pageSize=5 makes "6" invalid); change the parse to use base 10 (or use Number()) when converting queryValue to an integer in parsePageQuery so page numbers parse correctly (e.g., use Number.parseInt(String(queryValue ?? ''), 10) or Number(String(...)) and keep the same NaN/ <1 fallback logic).assets/vue/components/campaigns/ViewCampaignModal.vue (1)
189-207:⚠️ Potential issue | 🟠 MajorThis requeue message is reading and rewriting the wrong cutoff.
getMessage()is only used for requeueing, but it readsrepeatUntiland then floors the displayed time. If a campaign only hasrequeueUntil, the modal showsInvalid date; otherwise it can still show an earlier cutoff than the real one.💡 Minimal fix
function getMessage(schedule) { const interval = schedule.requeueInterval ?? schedule.repeatInterval if (!interval) return 'Invalid interval' + const rawEnd = schedule.requeueUntil ?? schedule.repeatUntil const end = new Date( - typeof schedule.repeatUntil === 'string' - ? schedule.repeatUntil.replace(' ', 'T') - : schedule.repeatUntil + typeof rawEnd === 'string' + ? rawEnd.replace(' ', 'T') + : rawEnd ) if (isNaN(end)) return 'Invalid date' - const minutes = end.getMinutes() - const flooredMinutes = minutes - (minutes % interval) - - const final = new Date(end) - final.setMinutes(flooredMinutes, 0, 0) - - return `every ${interval} minutes until ${final.toLocaleString()}` + return `every ${interval} minutes until ${end.toLocaleString()}` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/campaigns/ViewCampaignModal.vue` around lines 189 - 207, getMessage() is using repeatUntil to compute the cutoff even when the modal is for requeueing; change the date source to use schedule.requeueUntil ?? schedule.repeatUntil (mirror how interval is chosen from requeueInterval ?? repeatInterval), parse that value into end, validate it, floor minutes by interval, and then format the final date; update references in the function (getMessage, schedule.requeueUntil, schedule.repeatUntil, schedule.requeueInterval, schedule.repeatInterval) so the displayed cutoff reflects the actual requeue cutoff.
🧹 Nitpick comments (1)
assets/vue/components/base/CkEditorField.vue (1)
2-2: Consider making editor min-height configurable.
300pxis currently fixed at Line 2; exposing it as a prop would make this base component more reusable across forms/modals.Suggested refactor
- <div class="editor-field" :style="{ '--editor-min-height': `300px` }"> + <div class="editor-field" :style="{ '--editor-min-height': minHeight }"> ... const props = defineProps({ modelValue: { type: String, default: '' }, + minHeight: { + type: String, + default: '300px' + }, label: {Also applies to: 47-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/base/CkEditorField.vue` at line 2, Make the editor min-height configurable by adding a prop (e.g., editorMinHeight) on the CkEditorField component with a sensible default of "300px" and an appropriate type (String); replace the hardcoded style binding in the template (the div with class "editor-field" that sets '--editor-min-height') to use this prop, and update any other places in the same component that hardcode 300px (the editor initialization/configuration block referenced around the component methods/computed that set editor sizing) to reference the new prop so consumers can override it.
🤖 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/base/CkEditorField.vue`:
- Line 69: Replace the non-SSR-safe crypto.randomUUID() usage: import and call
Vue's useId() to generate a stable id and make fieldId reactive so it respects
changes to props.id; specifically, create a local id via useId() (e.g., const
generatedId = useId()) and expose fieldId as a computed value like computed(()
=> props.id || generatedId) instead of the current const fieldId = props.id ||
`ckeditor-${crypto.randomUUID()}` so IDs are stable across server/client and
update when props.id changes.
In `@assets/vue/components/campaigns/CampaignDirectory.vue`:
- Around line 463-472: fetchMailingLists() incorrectly flips the shared
isLoading flag causing the directory to leave loading state while
fetchCampaigns() may still be in flight; update the logic to use a separate
loading flag for lists (e.g., isLoadingMailingLists) or coordinate completion
(e.g., only clear isLoading in fetchCampaigns() or after both promises resolve).
Specifically, remove or stop setting isLoading.value = false in
fetchMailingLists(), introduce and manage a new ref like isLoadingMailingLists
(set true at start and false in its finally) or clear isLoading only once both
fetchCampaigns and fetchMailingLists have completed; reference functions
fetchMailingLists, fetchCampaigns and the isLoading ref when making the change.
In `@assets/vue/components/campaigns/ViewCampaignModal.vue`:
- Around line 105-116: The resend flow lacks a real owner so double-clicks and
unhandled failures occur; update ViewCampaignModal.vue by making handleResend
fully control local state: at the start of handleResend return early if
isResending is true, set isResending = true, clear resendErrorMessage, await the
resend call inside try/catch, on success emit an event like
$emit('resend-complete') and set isResending = false, on failure catch the
error, set resendErrorMessage to a user-friendly message (including
error.message), set isResending = false and emit $emit('resend-failed', error)
so the parent can react; ensure the Send button binding still uses isResending
and resendErrorMessage to disable the button and show the error.
In `@assets/vue/components/lists/ListDirectory.vue`:
- Around line 252-253: The current single call to listClient.getLists(0, 1000)
in ListDirectory.vue will silently truncate results; replace it with a
pagination loop that repeatedly calls listClient.getLists(offset, limit) (use
limit = 1000), collects items into a single array (e.g., allLists), and stops
when response?.pagination?.hasMore is false (or fallback to items.length !==
limit), then assign mailingLists.value = allLists; apply the same pagination
pattern to the analogous calls in CampaignEditView and CampaignDirectory where
listClient.getLists is used so no pages are dropped.
In `@assets/vue/views/CampaignEditView.vue`:
- Around line 273-275: The "Send test" button is a dead CTA with no click
handler; either wire it to a stub method or hide/disable it until implemented.
Add a method (e.g., sendTest or handleSendTest) inside the CampaignEditView
component and attach it to the button via `@click`="sendTest" to call the existing
test-send flow (or a placeholder that shows a toast/modal), or replace the
button with a disabled state (disabled attribute or v-if/v-show) until the full
flow exists; reference the button markup in CampaignEditView.vue and the
component methods section (sendTest / handleSendTest) when making the change.
- Around line 525-527: The sendFormat assignment calls .toLowerCase() without
ensuring format.sendFormat exists; update the sendFormat initializer (alongside
templateId and htmlFormated) to guard format.sendFormat before calling
toLowerCase — e.g. compute sendFormat by checking format.sendFormat (or using
String(format.sendFormat)) and falling back to 'html' when missing, so replace
sendFormat: format.sendFormat.toLowerCase() || 'html' with a guarded expression
like format.sendFormat ? String(format.sendFormat).toLowerCase() : 'html' (leave
templateId and htmlFormated logic unchanged).
- Around line 548-550: The form loads lists with a hard cap via
listClient.getLists(0, 1000) which misses lists beyond 1000 and breaks campaign
associations; replace this single-call with the same cursor-based pagination
used elsewhere (e.g., the pattern in CampaignDirectory.vue or
ListSubscribersView.vue) to fetch all pages until the cursor is null/empty,
accumulate results into the lists array, and then proceed with
campaignClient.getCampaign(campaignId.value) and associatedListResponse handling
so all linked lists are present and selectable in CampaignEditView.vue.
In `@src/Controller/CampaignsController.php`:
- Around line 18-21: The code calls $request->getSession()->get('auth_token')
directly which can throw SessionNotFoundException; update the render parameters
in CampaignsController so you first check for a session (e.g. $session =
$request->getSession() or use $request->hasSession()) and only call
get('auth_token') if the session exists, otherwise pass null (or an empty
string) as the api_token; apply the same guard to the second occurrence that
sets api_token (the other render call in this controller) so spa.html.twig
receives a safe fallback when no session is attached.
---
Duplicate comments:
In `@assets/vue/components/campaigns/CampaignDirectory.vue`:
- Around line 251-259: The mobile template's "Copy to draft" button condition is
using campaign.statusKey === 'set' while desktop expects 'sent', causing the
button to vanish on small screens; update the v-if to check for 'sent' instead
of 'set' in the CampaignDirectory.vue mobile branch (the button that calls
handleCopyToDraft(campaign.id) and uses
:disabled="isActionLoading(campaign.id)"), ensuring the conditional aligns with
the desktop logic so the action appears consistently.
- Around line 354-357: The parsePageQuery function is incorrectly passing
pageSize as the radix to Number.parseInt, causing wrong parsing (e.g.,
pageSize=5 makes "6" invalid); change the parse to use base 10 (or use Number())
when converting queryValue to an integer in parsePageQuery so page numbers parse
correctly (e.g., use Number.parseInt(String(queryValue ?? ''), 10) or
Number(String(...)) and keep the same NaN/ <1 fallback logic).
In `@assets/vue/components/campaigns/ViewCampaignModal.vue`:
- Around line 189-207: getMessage() is using repeatUntil to compute the cutoff
even when the modal is for requeueing; change the date source to use
schedule.requeueUntil ?? schedule.repeatUntil (mirror how interval is chosen
from requeueInterval ?? repeatInterval), parse that value into end, validate it,
floor minutes by interval, and then format the final date; update references in
the function (getMessage, schedule.requeueUntil, schedule.repeatUntil,
schedule.requeueInterval, schedule.repeatInterval) so the displayed cutoff
reflects the actual requeue cutoff.
---
Nitpick comments:
In `@assets/vue/components/base/CkEditorField.vue`:
- Line 2: Make the editor min-height configurable by adding a prop (e.g.,
editorMinHeight) on the CkEditorField component with a sensible default of
"300px" and an appropriate type (String); replace the hardcoded style binding in
the template (the div with class "editor-field" that sets '--editor-min-height')
to use this prop, and update any other places in the same component that
hardcode 300px (the editor initialization/configuration block referenced around
the component methods/computed that set editor sizing) to reference the new prop
so consumers can override it.
🪄 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: e4859246-a311-471f-9321-759c3a153d03
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
assets/router/index.jsassets/vue/api.jsassets/vue/components/base/CkEditorField.vueassets/vue/components/campaigns/CampaignDirectory.vueassets/vue/components/campaigns/ViewCampaignModal.vueassets/vue/components/lists/ListDirectory.vueassets/vue/views/CampaignEditView.vueconfig/services.ymlpackage.jsonsrc/Controller/CampaignsController.phpsrc/DependencyInjection/Compiler/RegisterEndpointsPass.phpsrc/PhpListFrontendBundle.php
💤 Files with no reviewable changes (1)
- config/services.yml
✅ Files skipped from review due to trivial changes (1)
- src/PhpListFrontendBundle.php
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- assets/vue/api.js
- assets/router/index.js
- src/DependencyInjection/Compiler/RegisterEndpointsPass.php
| set: (value) => emit('update:modelValue', value) | ||
| }) | ||
|
|
||
| const fieldId = props.id || `ckeditor-${crypto.randomUUID()}` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Vue 3.5.x, is useId()the recommended SSR-safe way to generate stable form control IDs, and what are the browser/security-context limitations ofcrypto.randomUUID()?
💡 Result:
Yes, for Vue 3.5.x, useId is the recommended SSR-safe way to generate stable form control IDs. It produces unique-per-app IDs (e.g., "v-1-2-3") that remain consistent between server and client renders, preventing hydration mismatches. This is explicitly stated in the official Vue documentation and Vue 3.5 announcement. crypto.randomUUID has these browser/security-context limitations: - Requires a secure context (HTTPS or localhost); unavailable over plain HTTP (non-localhost), causing errors like "crypto.randomUUID is not a function". - Broad browser support since 2022 (Chrome 92+, Firefox 95+, Safari 15.4+, etc.), but the secure context requirement limits it in development/non-HTTPS environments. - Not SSR-safe, as it generates different random values on server vs. client.
Citations:
- 1: feat(runtime-core): useId() vuejs/core#11404
- 2: https://vuejs.org/api/composition-api-helpers
- 3: https://blog.vuejs.org/posts/vue-3-5
- 4: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID
- 5: [Bug] latest release features the usage of crypto.randomUUID() which breaks when not using HTTPS Agenta-AI/agenta#3994
- 6: https://caniuse.com/mdn-api_crypto_randomuuid
🏁 Script executed:
cd /tmp && find . -type f -name "CkEditorField.vue" 2>/dev/null | head -20Repository: phpList/web-frontend
Length of output: 46
🏁 Script executed:
# Search for the file in common Vue project locations
find . -path "*/assets/vue/components/base/CkEditorField.vue" -o -path "*/src/components/CkEditorField.vue" 2>/dev/null | head -10Repository: phpList/web-frontend
Length of output: 110
🏁 Script executed:
# Broader search
fd -t f "CkEditorField" 2>/dev/nullRepository: phpList/web-frontend
Length of output: 108
🏁 Script executed:
cat -n assets/vue/components/base/CkEditorField.vue | head -100Repository: phpList/web-frontend
Length of output: 2354
🏁 Script executed:
cat package.json | grep -A 2 '"vue"'Repository: phpList/web-frontend
Length of output: 141
Use useId() instead of crypto.randomUUID() for SSR-safe ID generation.
Line 69 uses crypto.randomUUID(), which fails in non-HTTPS environments and produces different values on server vs. client (hydration mismatch). Additionally, fieldId is a constant, so it won't update if props.id changes. Vue 3.5.x provides useId() specifically for this—it generates stable, consistent IDs across renders.
Suggested fix
- import { computed } from 'vue'
+ import { computed, useId } from 'vue'
...
- const fieldId = props.id || `ckeditor-${crypto.randomUUID()}`
+ const generatedId = useId()
+ const fieldId = computed(() => props.id || `ckeditor-${generatedId}`)📝 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.
| const fieldId = props.id || `ckeditor-${crypto.randomUUID()}` | |
| import { computed, useId } from 'vue' | |
| // ... other code ... | |
| const generatedId = useId() | |
| const fieldId = computed(() => props.id || `ckeditor-${generatedId}`) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/base/CkEditorField.vue` at line 69, Replace the
non-SSR-safe crypto.randomUUID() usage: import and call Vue's useId() to
generate a stable id and make fieldId reactive so it respects changes to
props.id; specifically, create a local id via useId() (e.g., const generatedId =
useId()) and expose fieldId as a computed value like computed(() => props.id ||
generatedId) instead of the current const fieldId = props.id ||
`ckeditor-${crypto.randomUUID()}` so IDs are stable across server/client and
update when props.id changes.
| const fetchMailingLists = async () => { | ||
| try { | ||
| const response = await listClient.getLists(0, 1000) | ||
| mailingLists.value = Array.isArray(response?.items) ? response.items : [] | ||
| } catch (error) { | ||
| console.error('Failed to fetch mailing lists:', error) | ||
| mailingLists.value = [] | ||
| } finally { | ||
| isLoading.value = false | ||
| } |
There was a problem hiding this comment.
fetchMailingLists() is flipping the main directory loading flag.
fetchCampaigns() and fetchMailingLists() fire together on mount, but this finally always sets isLoading to false. If lists finish first, the table can drop its loading state and flash empty/error UI while campaigns are still in flight.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/campaigns/CampaignDirectory.vue` around lines 463 -
472, fetchMailingLists() incorrectly flips the shared isLoading flag causing the
directory to leave loading state while fetchCampaigns() may still be in flight;
update the logic to use a separate loading flag for lists (e.g.,
isLoadingMailingLists) or coordinate completion (e.g., only clear isLoading in
fetchCampaigns() or after both promises resolve). Specifically, remove or stop
setting isLoading.value = false in fetchMailingLists(), introduce and manage a
new ref like isLoadingMailingLists (set true at start and false in its finally)
or clear isLoading only once both fetchCampaigns and fetchMailingLists have
completed; reference functions fetchMailingLists, fetchCampaigns and the
isLoading ref when making the change.
| <button | ||
| type="button" | ||
| class="inline-flex items-center justify-center rounded-md bg-slate-900 px-4 py-2 text-sm font-medium text-white hover:bg-slate-800 disabled:cursor-not-allowed disabled:opacity-50" | ||
| :disabled="selectedMailingListIds.length === 0 || isResending" | ||
| @click="handleResend" | ||
| > | ||
| {{ isResending ? 'Sending...' : 'Send to lists' }} | ||
| </button> | ||
| </div> | ||
|
|
||
| <p v-if="resendErrorMessage" class="mt-2 text-sm text-red-600"> | ||
| {{ resendErrorMessage }} |
There was a problem hiding this comment.
Resend has no real pending/error owner right now.
The parent only passes @close in assets/vue/components/campaigns/CampaignDirectory.vue, Lines 317-324, so isResending stays false and resendErrorMessage never changes. That leaves this button open to double-click duplicate sends, and a failed resend has no UI path except an unhandled promise.
Also applies to: 155-162, 178-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/campaigns/ViewCampaignModal.vue` around lines 105 -
116, The resend flow lacks a real owner so double-clicks and unhandled failures
occur; update ViewCampaignModal.vue by making handleResend fully control local
state: at the start of handleResend return early if isResending is true, set
isResending = true, clear resendErrorMessage, await the resend call inside
try/catch, on success emit an event like $emit('resend-complete') and set
isResending = false, on failure catch the error, set resendErrorMessage to a
user-friendly message (including error.message), set isResending = false and
emit $emit('resend-failed', error) so the parent can react; ensure the Send
button binding still uses isResending and resendErrorMessage to disable the
button and show the error.
| const response = await listClient.getLists(0, 1000) | ||
| mailingLists.value = Array.isArray(response?.items) ? response.items : [] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are the pagination semantics of @tatevikgr/rest-api-client ListClient.getLists, and does calling getLists(0, 1000) return only the first page of results?
💡 Result:
No authoritative sources found for "@tatevikgr/rest-api-client" or "ListClient.getLists" method. Extensive searches across npm, GitHub, general web, and people profiles returned no matching package, repository, or documentation. The package appears to be private, unpublished, or non-existent publicly. Without source documentation, pagination semantics cannot be determined with high confidence. Common REST API pagination patterns (offset/limit) typically return up to the requested limit (e.g., getLists(0, 1000) would return the first 1000 items if available and server allows), not just one page, unless server-enforced page sizes exist.
Citations:
- 1: https://dataclient.io/rest/guides/pagination
- 2: https://ts-rest.com/client/fetch
- 3: https://dataclient.io/rest/api/resource
- 4: https://linkedin.com/in/tatev-suleymanyan-4167b12b0
- 5: https://www.npmjs.com/package/typed-rest-client
- 6: https://github.com/Microsoft/typed-rest-client
- 7: https://www.npmjs.com/package/typescript-rest-client
- 8: https://github.com/TatevikMirzoyan
- 9: https://github.com/tatevikh
🏁 Script executed:
# First, let's examine the api.js file to see how listClient is set up
head -100 assets/vue/api.jsRepository: phpList/web-frontend
Length of output: 1164
🏁 Script executed:
# Search for other uses of getLists in the codebase
rg "getLists" -A 3 -B 3Repository: phpList/web-frontend
Length of output: 2967
🏁 Script executed:
# Look for pagination patterns in the codebase
rg "pagination|page|offset|limit" -t vue -t js | head -30Repository: phpList/web-frontend
Length of output: 91
🏁 Script executed:
# Check if listClient is imported from an external package or defined locally
rg "@tatevikgr/rest-api-client|ListClient" -A 5Repository: phpList/web-frontend
Length of output: 3678
🏁 Script executed:
# Look at the full ListDirectory.vue file to understand context
wc -l assets/vue/components/lists/ListDirectory.vueRepository: phpList/web-frontend
Length of output: 113
🏁 Script executed:
# Check package.json to see if there's any documentation link
cat package.json | grep -A 5 -B 5 tatevikgrRepository: phpList/web-frontend
Length of output: 371
🏁 Script executed:
# Search for any README or docs that might mention pagination
find . -name "*.md" -o -name "*.MD" | head -20Repository: phpList/web-frontend
Length of output: 200
🏁 Script executed:
# Look for any test files or examples showing getLists usage
rg "getLists" -B 5 -A 10 -t js -t ts --max-count 50Repository: phpList/web-frontend
Length of output: 5206
🏁 Script executed:
# Check if there's any documentation about the API client in the codebase
fd -i readmeRepository: phpList/web-frontend
Length of output: 73
🏁 Script executed:
# Look at the actual ListDirectory.vue context more carefully
sed -n '240,270p' assets/vue/components/lists/ListDirectory.vueRepository: phpList/web-frontend
Length of output: 839
🏁 Script executed:
# Check if there's a homepage or repository link in package.json or related files
rg "tatevikgr" --type json -B 2 -A 2Repository: phpList/web-frontend
Length of output: 896
🏁 Script executed:
# Try to find any GitHub links or documentation references in the codebase
rg "github.com.*tatev" -iRepository: phpList/web-frontend
Length of output: 225
🏁 Script executed:
# Search for any comments in code that might explain the API usage
rg "getLists|pagination|limit|page" -B 3 -A 3 --type js --type tsRepository: phpList/web-frontend
Length of output: 27912
🏁 Script executed:
# Check if there are any API specifications or types defined locally
fd -e ts -e tsx | xargs grep -l "getLists" 2>/dev/null | head -5Repository: phpList/web-frontend
Length of output: 46
🏁 Script executed:
# Look for any node_modules or type definitions that might give us hints
find . -path "*/node_modules/*tatev*" -o -path "*/@tatevikgr*" 2>/dev/null | head -10Repository: phpList/web-frontend
Length of output: 46
Hard-coded 1000-item limit will silently truncate lists on larger installs.
The API supports pagination (evidenced by response?.pagination?.hasMore and nextCursor used elsewhere in CampaignDirectory.vue), but these calls fetch only the first 1000 items and discard the rest. This affects ListDirectory, CampaignEditView, and CampaignDirectory. Replace with a pagination loop similar to ListSubscribersExportPanel.vue:
Example pagination pattern from codebase
let offset = 0
const limit = 1000
let hasMore = true
const allLists = []
while (hasMore) {
const response = await listClient.getLists(offset, limit)
const items = Array.isArray(response?.items) ? response.items : []
allLists.push(...items)
hasMore = response?.pagination?.hasMore ?? (items.length === limit)
offset += limit
}
mailingLists.value = allLists🤖 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 252 - 253, The
current single call to listClient.getLists(0, 1000) in ListDirectory.vue will
silently truncate results; replace it with a pagination loop that repeatedly
calls listClient.getLists(offset, limit) (use limit = 1000), collects items into
a single array (e.g., allLists), and stops when response?.pagination?.hasMore is
false (or fallback to items.length !== limit), then assign mailingLists.value =
allLists; apply the same pagination pattern to the analogous calls in
CampaignEditView and CampaignDirectory where listClient.getLists is used so no
pages are dropped.
| <button type="button" class="rounded-md border border-slate-300 px-3 py-2 text-sm font-medium text-slate-700 hover:bg-slate-50"> | ||
| Send test | ||
| </button> |
There was a problem hiding this comment.
This "Send test" button is a dead CTA right now.
There’s no click handler behind it, so step 5 advertises a feature that does nothing. I’d either wire it up before merge or disable/hide it until the flow exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/views/CampaignEditView.vue` around lines 273 - 275, The "Send
test" button is a dead CTA with no click handler; either wire it to a stub
method or hide/disable it until implemented. Add a method (e.g., sendTest or
handleSendTest) inside the CampaignEditView component and attach it to the
button via `@click`="sendTest" to call the existing test-send flow (or a
placeholder that shows a toast/modal), or replace the button with a disabled
state (disabled attribute or v-if/v-show) until the full flow exists; reference
the button markup in CampaignEditView.vue and the component methods section
(sendTest / handleSendTest) when making the change.
| templateId: campaignValue?.template?.id ? String(campaignValue.template.id) : '', | ||
| sendFormat: format.sendFormat.toLowerCase() || 'html', | ||
| htmlFormated: format.htmlFormated !== null && format.htmlFormated !== undefined ? Boolean(format.htmlFormated) : true, |
There was a problem hiding this comment.
Guard missing sendFormat before calling .toLowerCase().
format defaults to {}, so a campaign without messageFormat.sendFormat will throw here and blank the whole edit screen.
💡 Minimal fix
- sendFormat: format.sendFormat.toLowerCase() || 'html',
+ sendFormat: typeof format.sendFormat === 'string'
+ ? format.sendFormat.toLowerCase()
+ : 'html',📝 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.
| templateId: campaignValue?.template?.id ? String(campaignValue.template.id) : '', | |
| sendFormat: format.sendFormat.toLowerCase() || 'html', | |
| htmlFormated: format.htmlFormated !== null && format.htmlFormated !== undefined ? Boolean(format.htmlFormated) : true, | |
| templateId: campaignValue?.template?.id ? String(campaignValue.template.id) : '', | |
| sendFormat: typeof format.sendFormat === 'string' | |
| ? format.sendFormat.toLowerCase() | |
| : 'html', | |
| htmlFormated: format.htmlFormated !== null && format.htmlFormated !== undefined ? Boolean(format.htmlFormated) : true, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/views/CampaignEditView.vue` around lines 525 - 527, The sendFormat
assignment calls .toLowerCase() without ensuring format.sendFormat exists;
update the sendFormat initializer (alongside templateId and htmlFormated) to
guard format.sendFormat before calling toLowerCase — e.g. compute sendFormat by
checking format.sendFormat (or using String(format.sendFormat)) and falling back
to 'html' when missing, so replace sendFormat: format.sendFormat.toLowerCase()
|| 'html' with a guarded expression like format.sendFormat ?
String(format.sendFormat).toLowerCase() : 'html' (leave templateId and
htmlFormated logic unchanged).
| const [campaignResponse, listResponse, associatedListResponse] = await Promise.all([ | ||
| campaignClient.getCampaign(campaignId.value), | ||
| listClient.getLists(0, 1000), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the CampaignEditView.vue file
fd -t f CampaignEditView.vueRepository: phpList/web-frontend
Length of output: 101
🏁 Script executed:
# Check the context around lines 548-550
if [ -f "assets/vue/views/CampaignEditView.vue" ]; then
sed -n '540,560p' assets/vue/views/CampaignEditView.vue
fiRepository: phpList/web-frontend
Length of output: 738
🏁 Script executed:
# Search for listClient definition and getLists usage
rg "listClient|getLists" assets/vue/views/CampaignEditView.vue -A 3 -B 3Repository: phpList/web-frontend
Length of output: 678
🏁 Script executed:
# Find where listClient is imported from
rg "import.*listClient|from.*listClient" assets/vue/views/ -A 2 -B 2Repository: phpList/web-frontend
Length of output: 485
🏁 Script executed:
# Find the api directory and files
fd -t f -p "api" assets/vue/Repository: phpList/web-frontend
Length of output: 81
🏁 Script executed:
# Look for index.ts or client definitions in api directory
ls -la assets/vue/api/Repository: phpList/web-frontend
Length of output: 126
🏁 Script executed:
# Search for getLists implementation
rg "getLists|listClient" assets/vue/api/ -A 5 -B 2Repository: phpList/web-frontend
Length of output: 119
🏁 Script executed:
# Find where listClient is defined/exported
rg "export.*listClient|const listClient" assets/vue/api/Repository: phpList/web-frontend
Length of output: 119
🏁 Script executed:
# Read the api.js file
wc -l assets/vue/api.jsRepository: phpList/web-frontend
Length of output: 84
🏁 Script executed:
# Read the api.js file in sections
cat -n assets/vue/api.jsRepository: phpList/web-frontend
Length of output: 1416
🏁 Script executed:
# Check package.json for the external package
cat package.json | grep -A 2 -B 2 "@tatevikgr"Repository: phpList/web-frontend
Length of output: 225
🏁 Script executed:
# Search for other getLists calls to understand pagination behavior
rg "getLists" --type vue --type ts --type js -B 2 -A 2Repository: phpList/web-frontend
Length of output: 91
🏁 Script executed:
# Look for any constant definitions or limits related to lists
rg "1000|LIST.*LIMIT|MAX.*LIST" assets/vue/ -iRepository: phpList/web-frontend
Length of output: 578
🏁 Script executed:
# Search for pagination or load more patterns
rg "pagination|loadMore|load.*more|hasMore|offset" assets/vue/ -i -C 2Repository: phpList/web-frontend
Length of output: 14288
🏁 Script executed:
# Look at the context around the mailingLists usage in the form
sed -n '540,620p' assets/vue/views/CampaignEditView.vueRepository: phpList/web-frontend
Length of output: 2665
🏁 Script executed:
# Search for any TODO or FIXME comments about lists
rg "TODO|FIXME|BUG|HACK" assets/vue/views/CampaignEditView.vue -iRepository: phpList/web-frontend
Length of output: 46
Hard cap at 1000 lists prevents editing campaigns linked beyond that threshold.
The getLists(0, 1000) call fetches only the first 1000 lists. If campaigns are already linked to lists beyond that, those list associations won't appear as checked in this form, making it impossible to view or modify them. The rest of the codebase handles pagination properly (e.g., in CampaignDirectory.vue, ListSubscribersView.vue), so this should use the same cursor-based approach to load all available lists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/views/CampaignEditView.vue` around lines 548 - 550, The form loads
lists with a hard cap via listClient.getLists(0, 1000) which misses lists beyond
1000 and breaks campaign associations; replace this single-call with the same
cursor-based pagination used elsewhere (e.g., the pattern in
CampaignDirectory.vue or ListSubscribersView.vue) to fetch all pages until the
cursor is null/empty, accumulate results into the lists array, and then proceed
with campaignClient.getCampaign(campaignId.value) and associatedListResponse
handling so all linked lists are present and selectable in CampaignEditView.vue.
| return $this->render('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.
🧩 Analysis chain
🌐 Web query:
In Symfony HttpFoundation, does Request::getSession() throw when no session exists, and is Request::hasSession() the right guard before reading from it?
💡 Result:
Yes, in current Symfony versions (6.x as of 2026), Request::getSession throws a SessionNotFoundException when no session exists. This behavior was deprecated in Symfony 4.1 (returning null with deprecation warning) and changed to throw an exception in Symfony 5.0. Request::hasSession is the correct guard to use before calling getSession, as recommended in official documentation and changelogs. Example: if ($request->hasSession) { $session = $request->getSession; // use $session }
Citations:
- 1: https://symfony.com/blog/new-in-symfony-4-1-session-improvements
- 2: https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/HttpFoundation/RequestStack.php
- 3: https://github.com/symfony/http-foundation/blob/6.3/Request.php
- 4: [HttpFoundation] Fix consistency in sessions not found exceptions symfony/symfony#40114
- 5: Request::getSession return type should not be null symfony/symfony#26539
Guard getSession() before reading auth_token.
Request::getSession() throws a SessionNotFoundException when no session is attached, so anonymous or expired hits can cause a 500 error before the SPA even renders. Since spa.html.twig already defaults api_token to an empty string when absent, passing null is the safer fallback.
💡 Fix for both locations
return $this->render('spa.html.twig', [
'page' => 'Campaigns',
- 'api_token' => $request->getSession()->get('auth_token'),
+ 'api_token' => $request->hasSession() ? $request->getSession()->get('auth_token') : null,
'api_base_url' => $this->getParameter('api_base_url'),
]);
@@
return $this->render('spa.html.twig', [
'page' => sprintf('Edit Campaign #%d', $campaignId),
- 'api_token' => $request->getSession()->get('auth_token'),
+ 'api_token' => $request->hasSession() ? $request->getSession()->get('auth_token') : null,
'api_base_url' => $this->getParameter('api_base_url'),
]);Also applies to: Lines 28-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Controller/CampaignsController.php` around lines 18 - 21, The code calls
$request->getSession()->get('auth_token') directly which can throw
SessionNotFoundException; update the render parameters in CampaignsController so
you first check for a session (e.g. $session = $request->getSession() or use
$request->hasSession()) and only call get('auth_token') if the session exists,
otherwise pass null (or an empty string) as the api_token; apply the same guard
to the second occurrence that sets api_token (the other render call in this
controller) so spa.html.twig receives a safe fallback when no session is
attached.
2 participants
Summary by CodeRabbit
New Features
Chores
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!