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 a Campaigns feature across backend and frontend. Backend: new CampaignsController routes under /campaigns, a DI compiler pass (RegisterEndpointsPass) to auto-register REST endpoint classes, bundle registration of that pass, and Twig namespace prepending. Frontend: router routes for campaigns, new API client exports for campaigns/statistics/templates/messages, CampaignsView/CampaignEditView and supporting components (CampaignDirectory, ViewCampaignModal, CkEditorField), admin topbar search and sidebar tweaks. Dependency updates: rest-api-client v2, CKEditor packages, and webpack-cli version adjustment. Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant VueRouter as Router
participant CampaignsView as View
participant Directory as Directory
participant API_Campaign as CampaignAPI
participant API_Stats as StatsAPI
participant API_Lists as ListAPI
User->>Browser: Navigate to /campaigns
Browser->>Router: resolve route
Router->>View: mount CampaignsView
View->>Directory: mount CampaignDirectory
Directory->>API_Campaign: fetch campaigns (cursor pagination)
API_Campaign-->>Directory: campaigns list
Directory->>API_Stats: fetch aggregated statistics
API_Stats-->>Directory: statistics map
Directory->>Browser: render list (pagination/filters)
User->>Browser: click "View" on campaign
Browser->>API_Campaign: fetch campaign details
API_Campaign-->>Browser: campaign details
Browser->>Directory: open ViewCampaignModal
alt Actions requiring lists
Directory->>API_Lists: fetch mailing lists for campaign
API_Lists-->>Directory: lists
Directory->>API_Campaign: perform action (resend/delete/suspend)
API_Campaign-->>Directory: action result
Directory->>API_Campaign: refresh campaigns
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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
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
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
assets/vue/views/CampaignEditView.vue (1)
625-652:⚠️ Potential issue | 🟠 MajorStill capped to the first 1000 lists.
Both the create path on Line 627 and the edit path on Line 651 only load the first page. Campaigns linked to lists beyond that cap still won't show up as selected here, so those associations can't be reviewed or edited from this screen.
🤖 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 625 - 652, The create and edit flows only call listClient.getLists(0, 1000) so mailingLists and associatedListIds may miss lists beyond the first page; update the code around the calls to listClient.getLists (both in the isCreateMode branch where listResponse is used and in the Promise.all where listResponse is fetched) to fetch all pages instead of a single page—either implement a small helper (e.g., fetchAllLists or listClient.getAllLists) that repeatedly requests getLists(page, size) until no more items, or call the API with proper pagination parameters to accumulate all items into mailingLists (and then derive associatedListIds/selectedListIds) before returning or proceeding with campaignResponse processing; ensure you replace uses of listResponse?.items with the fully accumulated array.
🤖 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/views/CampaignEditView.vue`:
- Around line 812-823: The route replace can trigger loadCampaignData() before
lists are synced, causing selectedListIds to be populated from
getListsByMessage() too early; move the await syncLists() so it completes before
calling router.replace() (or delay the route change until after associations are
persisted) in the create/save flow—update the block around createdCampaignId
(using router.replace, syncLists, loadCampaignData, selectedListIds,
getListsByMessage) and make the same change for the other occurrence around
lines 957-963 so the route watcher always sees up-to-date list associations.
- Around line 924-925: The catch block currently assumes error.message is a
string and does saveError.value = error?.message.slice(0, 100), which can throw
if a non-Error is thrown; update the assignment for saveError.value to guard the
message (e.g., check typeof error?.message === 'string' and slice that,
otherwise use String(error) or a fallback message) and keep the console.error
call but ensure it logs the raw error as well (e.g., console.error('Failed to
send test campaign:', error)). This change touches the saveError.value
assignment and the error logging around the test-send catch in
CampaignEditView.vue.
- Around line 871-873: After successfully calling
campaignClient.updateCampaignStatus(currentCampaignId, 'submitted') update the
local campaign model so canQueueCampaign (derived from
campaign.value?.messageMetadata?.status) reflects the new state; specifically,
set campaign.value.messageMetadata.status = 'submitted' (or update the relevant
messageMetadata/status field) and update any reactive flags like saveSuccess
accordingly so the Queue button disables without a reload. Reference
campaignClient.updateCampaignStatus, canQueueCampaign,
campaign.value?.messageMetadata?.status, and saveSuccess in your change.
- Around line 892-918: sendTestCampaign currently sends the persisted campaign
on the server instead of the on-screen draft; before calling
campaignClient.testSendCampaign(currentCampaignId, recipients) ensure the
current draft is saved/updated first by invoking the existing save function
(e.g., saveCampaign or saveCampaignDraft) with form.value and awaiting it (or,
if API supports it, pass the draft payload to campaignClient.testSendCampaign as
an extra argument), then proceed to call campaignClient.testSendCampaign using
activeCampaignId and recipients so the test uses the latest form.value content
and selections.
---
Duplicate comments:
In `@assets/vue/views/CampaignEditView.vue`:
- Around line 625-652: The create and edit flows only call
listClient.getLists(0, 1000) so mailingLists and associatedListIds may miss
lists beyond the first page; update the code around the calls to
listClient.getLists (both in the isCreateMode branch where listResponse is used
and in the Promise.all where listResponse is fetched) to fetch all pages instead
of a single page—either implement a small helper (e.g., fetchAllLists or
listClient.getAllLists) that repeatedly requests getLists(page, size) until no
more items, or call the API with proper pagination parameters to accumulate all
items into mailingLists (and then derive associatedListIds/selectedListIds)
before returning or proceeding with campaignResponse processing; ensure you
replace uses of listResponse?.items with the fully accumulated array.
🪄 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: aedc3c59-8da5-470d-84bf-8201a2b6fca4
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
assets/router/index.jsassets/vue/components/base/CkEditorField.vueassets/vue/components/sidebar/AppSidebar.vueassets/vue/layouts/AdminLayout.vueassets/vue/views/CampaignEditView.vuecomposer.jsonpackage.jsonsrc/Controller/CampaignsController.php
✅ Files skipped from review due to trivial changes (1)
- composer.json
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- assets/router/index.js
- assets/vue/components/base/CkEditorField.vue
- src/Controller/CampaignsController.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/DependencyInjection/PhpListFrontendExtension.php`:
- Around line 34-41: The class PhpListFrontendExtension defines prepend() but
doesn't implement PrependExtensionInterface so Symfony won't call it; update the
class declaration to implement
Symfony\Component\DependencyInjection\Extension\PrependExtensionInterface (add
the corresponding use statement) and keep the existing public function
prepend(ContainerBuilder $container): void signature so Symfony will invoke it
and the Twig path namespace registration executes.
🪄 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: c9d07401-c0fe-41a3-be2d-ead1dd236348
📒 Files selected for processing (6)
src/Controller/AuthController.phpsrc/Controller/CampaignsController.phpsrc/Controller/DashboardController.phpsrc/Controller/ListsController.phpsrc/Controller/SubscribersController.phpsrc/DependencyInjection/PhpListFrontendExtension.php
✅ Files skipped from review due to trivial changes (4)
- src/Controller/SubscribersController.php
- src/Controller/DashboardController.php
- src/Controller/AuthController.php
- src/Controller/CampaignsController.php
TatevikGr
force-pushed
the
feat/campaigns
branch
from
d2ac94c to
30c0390
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/DependencyInjection/PhpListFrontendExtension.php (1)
34-41:⚠️ Potential issue | 🔴 Critical
prepend()won’t run unless the extension implements the Symfony interface.Nice addition here, but this won’t execute in practice until Line 14 implements
PrependExtensionInterface; otherwise Twig namespace prepending is skipped and@PhpListFrontend/...templates can fail at runtime.Suggested fix
use Symfony\Component\DependencyInjection\ContainerBuilder; +use Symfony\Component\DependencyInjection\Extension\PrependExtensionInterface; use Symfony\Component\DependencyInjection\Loader\YamlFileLoader; use Symfony\Component\HttpKernel\DependencyInjection\Extension; -class PhpListFrontendExtension extends Extension +class PhpListFrontendExtension extends Extension implements PrependExtensionInterface#!/bin/bash set -euo pipefail # Verify class declaration/interface rg -nP 'class\s+PhpListFrontendExtension\s+extends\s+Extension(\s+implements\s+[^{]+)?' src/DependencyInjection/PhpListFrontendExtension.php rg -n 'PrependExtensionInterface' src/DependencyInjection/PhpListFrontendExtension.php # Verify namespaced template usage that depends on prepend() behavior rg -n '@PhpListFrontend/' templates src/Controller🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DependencyInjection/PhpListFrontendExtension.php` around lines 34 - 41, The prepend() method in PhpListFrontendExtension will never be called unless the class implements Symfony\Component\DependencyInjection\Extension\PrependExtensionInterface; update the class declaration for PhpListFrontendExtension to implement PrependExtensionInterface and add the corresponding use/import, and ensure the prepend(ContainerBuilder $container): void signature matches the interface; this will allow the prepend() method to run and register the Twig path used by the '@PhpListFrontend/...' templates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/DependencyInjection/PhpListFrontendExtension.php`:
- Around line 34-41: The prepend() method in PhpListFrontendExtension will never
be called unless the class implements
Symfony\Component\DependencyInjection\Extension\PrependExtensionInterface;
update the class declaration for PhpListFrontendExtension to implement
PrependExtensionInterface and add the corresponding use/import, and ensure the
prepend(ContainerBuilder $container): void signature matches the interface; this
will allow the prepend() method to run and register the Twig path used by the
'@PhpListFrontend/...' templates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9cd18887-9d5b-4095-bc8b-04aa22725975
📒 Files selected for processing (8)
src/Controller/AuthController.phpsrc/Controller/CampaignsController.phpsrc/Controller/DashboardController.phpsrc/Controller/ListsController.phpsrc/Controller/SubscribersController.phpsrc/DependencyInjection/PhpListFrontendExtension.phptemplates/auth/login.html.twigtemplates/spa.html.twig
✅ Files skipped from review due to trivial changes (7)
- src/Controller/DashboardController.php
- src/Controller/SubscribersController.php
- templates/auth/login.html.twig
- templates/spa.html.twig
- src/Controller/ListsController.php
- src/Controller/CampaignsController.php
- src/Controller/AuthController.php
TatevikGr
force-pushed
the
feat/campaigns
branch
2 times, most recently
from
8ac6043 to
a83d791
Compare
TatevikGr
force-pushed
the
feat/campaigns
branch
from
4c6ef7b to
39f84c1
Compare
TatevikGr
force-pushed
the
feat/campaigns
branch
from
6996ef2 to
4efd3a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
templates/base.html.twig (1)
7-12: Swap hard-coded build filenames for Encore entrypoint helpers.The template is hard-coding
app.css,styles.css,runtime.js, andapp.jsinstead of using Encore's built-in helpers likeencore_entry_script_tags()andencore_entry_link_tags(). Since your Encore config already defines.addEntry()and.addStyleEntry(), leaning on those helpers would keep the template in sync automatically if the build output changes (e.g., different entry names, versioning, or chunk splits down the line). It's an easy refactor that saves future headaches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/base.html.twig` around lines 7 - 12, The template currently hard-codes build filenames (app.css, styles.css, runtime.js, app.js); replace those with Encore entry helpers so asset names/versioning/chunking stay correct—use encore_entry_link_tags() for the CSS entries (e.g., the style entry named "styles" and the main entry "app" if those are the Encore entries) and encore_entry_script_tags() for the JS entries (the "app" entry which will include runtime/chunks automatically) in the base template (replace the direct asset(...) calls in templates/base.html.twig with the corresponding encore_entry_link_tags('styles') and encore_entry_link_tags('app') as needed and encore_entry_script_tags('app')).
🤖 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/ViewCampaignModal.vue`:
- Around line 187-205: The getMessage helper is using schedule.repeatUntil for
the requeue summary causing "Invalid date" when a campaign provides
schedule.requeueUntil; update getMessage to prefer schedule.requeueUntil when
schedule.requeueInterval is present (fall back to schedule.repeatUntil only if
requeueUntil is missing) and parse that chosen timestamp the same way it
currently parses repeatUntil; adjust references to interval detection (using
schedule.requeueInterval ?? schedule.repeatInterval) and to the date variable so
final toLocaleString() reflects requeueUntil when applicable.
In `@assets/vue/views/CampaignEditView.vue`:
- Around line 974-983: The watcher on route.query.section only updates
currentStep when resolveStepFromRouteValue returns a truthy nextStepValue,
leaving currentStep unchanged for absent/invalid sections; change the watcher
(the callback using resolveStepFromRouteValue and currentStep) so that when
nextStepValue is falsy you explicitly reset currentStep.value to the initial
step (e.g., 1 or the component's default step) — i.e., set currentStep.value = 1
(or the existing initialStep variable) when sectionValue is missing/invalid so
navigating to /campaigns/create always resets the wizard.
- Around line 757-783: When some associate/dissociate calls succeed and others
fail, the code currently throws before updating associatedListIds so the editor
stays out-of-sync; after running the two loops that build results, compute the
actual post-operation set from the results (apply each fulfilled result: include
listId for successful associate, exclude listId for successful dissociate, or
fallback to nextIds for ambiguous cases) and assign that computed set to
associatedListIds.value before throwing the Error; update the block that
inspects results (symbols: results, toAdd, toRemove,
listMessagesClient.associateMessageWithList,
listMessagesClient.dissociateMessageFromList, associatedListIds, nextIds) so
partial successes are reflected locally even when some operations failed.
In `@src/Controller/AuthController.php`:
- Line 84: Replace the concatenated raw exception message with a stable log
message and safe metadata: in AuthController where you currently call
$this->logger->error('Unable to load current user: ' . $e->getMessage()), change
it to $this->logger->error('Unable to load current user', ['exception_class' =>
get_class($e), 'exception_code' => $e->getCode(), /* optional safe context like
'user_id' => $userId */]); this keeps the message constant and records
non-sensitive diagnostics (exception class/code and any safe IDs) instead of
exposing $e->getMessage().
In `@src/DependencyInjection/PhpListFrontendExtension.php`:
- Around line 58-82: The bundle is prepending a full security configuration via
the call to prependExtensionConfig('security', ...) inside
PhpListFrontendExtension which overrides app-level firewalls/providers; remove
that entire prependExtensionConfig('security', [...]) block from the
PhpListFrontendExtension class so the bundle no longer injects global security
rules and the application can define security in config/packages/security.yaml
as intended.
---
Nitpick comments:
In `@templates/base.html.twig`:
- Around line 7-12: The template currently hard-codes build filenames (app.css,
styles.css, runtime.js, app.js); replace those with Encore entry helpers so
asset names/versioning/chunking stay correct—use encore_entry_link_tags() for
the CSS entries (e.g., the style entry named "styles" and the main entry "app"
if those are the Encore entries) and encore_entry_script_tags() for the JS
entries (the "app" entry which will include runtime/chunks automatically) in the
base template (replace the direct asset(...) calls in templates/base.html.twig
with the corresponding encore_entry_link_tags('styles') and
encore_entry_link_tags('app') as needed and encore_entry_script_tags('app')).
🪄 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: 4f223316-f4ae-482d-a600-f7d6d2d7470d
📒 Files selected for processing (18)
README.mdassets/vue/api.jsassets/vue/components/base/CkEditorField.vueassets/vue/components/campaigns/CampaignDirectory.vueassets/vue/components/campaigns/ViewCampaignModal.vueassets/vue/components/lists/ListDirectory.vueassets/vue/components/subscribers/SubscriberDirectory.vueassets/vue/views/CampaignEditView.vuesrc/Controller/AuthController.phpsrc/Controller/CampaignsController.phpsrc/Controller/DashboardController.phpsrc/Controller/ListsController.phpsrc/Controller/SubscribersController.phpsrc/DependencyInjection/PhpListFrontendExtension.phpsrc/Security/SessionAuthenticator.phptemplates/auth/login.html.twigtemplates/base.html.twigtemplates/spa.html.twig
✅ Files skipped from review due to trivial changes (5)
- README.md
- templates/auth/login.html.twig
- templates/spa.html.twig
- assets/vue/components/subscribers/SubscriberDirectory.vue
- src/Controller/ListsController.php
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Controller/DashboardController.php
- src/Controller/SubscribersController.php
- assets/vue/components/lists/ListDirectory.vue
- assets/vue/api.js
- assets/vue/components/base/CkEditorField.vue
- src/Controller/CampaignsController.php
Comment on lines
+187
to
+205
| function getMessage(schedule) { | ||
| const interval = schedule.requeueInterval ?? schedule.repeatInterval | ||
| if (!interval) return 'Invalid interval' | ||
|
|
||
| const end = new Date( | ||
| typeof schedule.repeatUntil === 'string' | ||
| ? schedule.repeatUntil.replace(' ', 'T') | ||
| : schedule.repeatUntil | ||
| ) | ||
|
|
||
| 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()}` |
There was a problem hiding this comment.
Use requeueUntil for the requeue summary.
This helper is rendered for requeue schedules, but it always parses schedule.repeatUntil. Campaigns that set requeueUntil without repeatUntil will show Invalid date even though the requeue window is valid.
💡 Minimal fix
function getMessage(schedule) {
const interval = schedule.requeueInterval ?? schedule.repeatInterval
if (!interval) return 'Invalid interval'
+ const untilValue = schedule.requeueUntil ?? schedule.repeatUntil
const end = new Date(
- typeof schedule.repeatUntil === 'string'
- ? schedule.repeatUntil.replace(' ', 'T')
- : schedule.repeatUntil
+ typeof untilValue === 'string'
+ ? untilValue.replace(' ', 'T')
+ : untilValue
)📝 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
| function getMessage(schedule) { | |
| const interval = schedule.requeueInterval ?? schedule.repeatInterval | |
| if (!interval) return 'Invalid interval' | |
| const end = new Date( | |
| typeof schedule.repeatUntil === 'string' | |
| ? schedule.repeatUntil.replace(' ', 'T') | |
| : schedule.repeatUntil | |
| ) | |
| 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()}` | |
| function getMessage(schedule) { | |
| const interval = schedule.requeueInterval ?? schedule.repeatInterval | |
| if (!interval) return 'Invalid interval' | |
| const untilValue = schedule.requeueUntil ?? schedule.repeatUntil | |
| const end = new Date( | |
| typeof untilValue === 'string' | |
| ? untilValue.replace(' ', 'T') | |
| : untilValue | |
| ) | |
| 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()}` | |
| } |
🤖 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 187 -
205, The getMessage helper is using schedule.repeatUntil for the requeue summary
causing "Invalid date" when a campaign provides schedule.requeueUntil; update
getMessage to prefer schedule.requeueUntil when schedule.requeueInterval is
present (fall back to schedule.repeatUntil only if requeueUntil is missing) and
parse that chosen timestamp the same way it currently parses repeatUntil; adjust
references to interval detection (using schedule.requeueInterval ??
schedule.repeatInterval) and to the date variable so final toLocaleString()
reflects requeueUntil when applicable.
Comment on lines
+757
to
+783
| const results = [] | ||
|
|
||
| for (const listId of toAdd) { | ||
| try { | ||
| const result = await listMessagesClient.associateMessageWithList(currentCampaignId, listId) | ||
| results.push({ status: 'fulfilled', value: result }) | ||
| } catch (error) { | ||
| results.push({ status: 'rejected', reason: error, type: 'add', listId }) | ||
| } | ||
| } | ||
|
|
||
| for (const listId of toRemove) { | ||
| try { | ||
| const result = await listMessagesClient.dissociateMessageFromList(currentCampaignId, listId) | ||
| results.push({ status: 'fulfilled', value: result }) | ||
| } catch (error) { | ||
| results.push({ status: 'rejected', reason: error, type: 'remove', listId }) | ||
| } | ||
| } | ||
|
|
||
| const failedOperations = results.filter((result) => result.status === 'rejected') | ||
|
|
||
| if (failedOperations.length > 0) { | ||
| throw new Error(`Failed to update ${failedOperations.length} list association(s).`) | ||
| } | ||
|
|
||
| associatedListIds.value = [...nextIds] |
There was a problem hiding this comment.
Reconcile local list state after partial sync failures.
If one association call succeeds and a later one fails, this throws while associatedListIds still points at the old snapshot. The next save can then replay already-applied mutations and keep the editor out of sync with the server.
💡 Minimal fix
const failedOperations = results.filter((result) => result.status === 'rejected')
if (failedOperations.length > 0) {
+ const latestAssociations = await listMessagesClient.getListsByMessage(currentCampaignId)
+ associatedListIds.value = Array.isArray(latestAssociations?.items)
+ ? latestAssociations.items
+ .map((list) => Number(list.id))
+ .filter((id) => Number.isFinite(id))
+ : [...associatedListIds.value]
throw new Error(`Failed to update ${failedOperations.length} list association(s).`)
}
associatedListIds.value = [...nextIds]🤖 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 757 - 783, When some
associate/dissociate calls succeed and others fail, the code currently throws
before updating associatedListIds so the editor stays out-of-sync; after running
the two loops that build results, compute the actual post-operation set from the
results (apply each fulfilled result: include listId for successful associate,
exclude listId for successful dissociate, or fallback to nextIds for ambiguous
cases) and assign that computed set to associatedListIds.value before throwing
the Error; update the block that inspects results (symbols: results, toAdd,
toRemove, listMessagesClient.associateMessageWithList,
listMessagesClient.dissociateMessageFromList, associatedListIds, nextIds) so
partial successes are reflected locally even when some operations failed.
Comment on lines
+974
to
+983
| watch( | ||
| () => route.query.section, | ||
| (sectionValue) => { | ||
| const nextStepValue = resolveStepFromRouteValue(sectionValue) | ||
| if (nextStepValue && nextStepValue !== currentStep.value) { | ||
| currentStep.value = nextStepValue | ||
| } | ||
| }, | ||
| { immediate: true } | ||
| ) |
There was a problem hiding this comment.
Reset the wizard to step 1 when section is absent.
Right now a missing/invalid section leaves currentStep untouched. Since assets/router/index.js:16-17 reuse this component for both create and edit routes, navigating from a deep step to /campaigns/create can reopen the new campaign on the previous step.
💡 Minimal fix
watch(
() => route.query.section,
(sectionValue) => {
- const nextStepValue = resolveStepFromRouteValue(sectionValue)
- if (nextStepValue && nextStepValue !== currentStep.value) {
+ const nextStepValue = resolveStepFromRouteValue(sectionValue) ?? 1
+ if (nextStepValue !== currentStep.value) {
currentStep.value = nextStepValue
}
},
{ immediate: 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 974 - 983, The watcher on
route.query.section only updates currentStep when resolveStepFromRouteValue
returns a truthy nextStepValue, leaving currentStep unchanged for absent/invalid
sections; change the watcher (the callback using resolveStepFromRouteValue and
currentStep) so that when nextStepValue is falsy you explicitly reset
currentStep.value to the initial step (e.g., 1 or the component's default step)
— i.e., set currentStep.value = 1 (or the existing initialStep variable) when
sectionValue is missing/invalid so navigating to /campaigns/create always resets
the wizard.
| try { | ||
| $user = $this->authClient->getSessionUser(); | ||
| } catch (Exception | GuzzleException $e) { | ||
| $this->logger->error('Unable to load current user: ' . $e->getMessage()); |
There was a problem hiding this comment.
Avoid logging raw exception messages at error level.
At Line 84, concatenating $e->getMessage() can leak internal/backend details into logs. Keep the error message stable and log only safe metadata.
🔧 Proposed safe logging change
- $this->logger->error('Unable to load current user: ' . $e->getMessage());
+ $this->logger->error('Unable to load current user.', [
+ 'exception_class' => $e::class,
+ ]);📝 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
| $this->logger->error('Unable to load current user: ' . $e->getMessage()); | |
| $this->logger->error('Unable to load current user.', [ | |
| 'exception_class' => $e::class, | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Controller/AuthController.php` at line 84, Replace the concatenated raw
exception message with a stable log message and safe metadata: in AuthController
where you currently call $this->logger->error('Unable to load current user: ' .
$e->getMessage()), change it to $this->logger->error('Unable to load current
user', ['exception_class' => get_class($e), 'exception_code' => $e->getCode(),
/* optional safe context like 'user_id' => $userId */]); this keeps the message
constant and records non-sensitive diagnostics (exception class/code and any
safe IDs) instead of exposing $e->getMessage().
Comment on lines
+58
to
+82
| $container->prependExtensionConfig('security', [ | ||
| 'providers' => [ | ||
| 'in_memory' => ['memory' => null], | ||
| ], | ||
| 'firewalls' => [ | ||
| 'api' => [ | ||
| 'pattern' => '^/api/v2', | ||
| 'security' => false, | ||
| ], | ||
| 'main' => [ | ||
| 'lazy' => true, | ||
| 'provider' => 'in_memory', | ||
| 'pattern' => '^/', | ||
| 'custom_authenticators' => [ | ||
| 'PhpList\\WebFrontend\\Security\\SessionAuthenticator', | ||
| ], | ||
| 'entry_point' => 'PhpList\\WebFrontend\\Security\\SessionAuthenticator', | ||
| ], | ||
| ], | ||
| 'access_control' => [ | ||
| ['path' => '^/login', 'roles' => 'PUBLIC_ACCESS'], | ||
| ['path' => '^/api/v2', 'roles' => 'PUBLIC_ACCESS'], | ||
| ['path' => '^/', 'roles' => 'ROLE_ADMIN'], | ||
| ], | ||
| ]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Inspect the prepended catch-all rules in this extension:"
sed -n '58,82p' src/DependencyInjection/PhpListFrontendExtension.php
echo
echo "2) Find application-level security config that would be evaluated after them:"
fd -i 'security*.yaml' config src
fd -i 'security*.yml' config src
rg -n -C2 "firewalls:|access_control:|providers:" config src
echo
echo "3) Inspect controller route declarations to identify a narrower bundle prefix:"
rg -n "Route\\(" src/Controller configRepository: phpList/web-frontend
Length of output: 3727
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Check the full content of config/packages/security.yaml:"
cat config/packages/security.yaml
echo
echo "2) Check if there are any comments or docs about why config is prepended:"
rg -n "prependExtensionConfig" src/DependencyInjection/ -B2 -A2
echo
echo "3) Look for any bundle configuration or routing prefix:"
rg -n "bundle|prefix|namespace" src/DependencyInjection/PhpListFrontendExtension.phpRepository: phpList/web-frontend
Length of output: 2366
Remove prepended security rules—keep them app-level only.
The extension duplicates the entire security config that already exists in config/packages/security.yaml. Prepending it from the bundle means the extension's rules take precedence and prevent the application from adding custom firewalls, providers, or access rules that would need to coexist. This breaks composability.
Unlike the Twig and framework asset configs (which are bundle-specific), security is global application-level config and shouldn't be hardcoded in the extension. Remove the prependExtensionConfig('security', ...) block entirely and let the application define these rules.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/DependencyInjection/PhpListFrontendExtension.php` around lines 58 - 82,
The bundle is prepending a full security configuration via the call to
prependExtensionConfig('security', ...) inside PhpListFrontendExtension which
overrides app-level firewalls/providers; remove that entire
prependExtensionConfig('security', [...]) block from the
PhpListFrontendExtension class so the bundle no longer injects global security
rules and the application can define security in config/packages/security.yaml
as intended.
55c748f to
d4b92f4
Compare
TatevikGr
force-pushed
the
feat/campaigns
branch
11 times, most recently
from
60293be to
bb2b97b
Compare
TatevikGr
force-pushed
the
feat/campaigns
branch
from
bb2b97b to
cace371
Compare
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
Style / Layout
Chores
Documentation
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!