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:
📝 WalkthroughWalkthroughThis PR adds a Vue 3 SPA frontend (router, many Vue components, layouts, views, composables, and tests), integrates Tailwind/PostCSS/Vite tooling, and wires a shared REST API client. On the backend it introduces Symfony controllers (Dashboard, Subscribers, Lists, Campaigns), session-based authentication (SessionAuthenticator, AuthGateSubscriber), API session wiring (ApiSessionListener), DI changes (compiler pass, extension), security config, and removes the deprecated ApiClient. Numerous tests and CI/frontend build updates are included. Sequence Diagram(s)sequenceDiagram
participant Browser
participant Vue as Vue Router/App
participant Server as Symfony
participant AuthGate as AuthGateSubscriber
participant SessionAuth as SessionAuthenticator
participant Session as Session Storage
Browser->>Vue: Navigate to protected route
Vue->>Server: HTTP request
Server->>AuthGate: Kernel Request event
AuthGate->>Session: Check auth_token
alt no auth_token
AuthGate->>Browser: Redirect to /login
else has auth_token
AuthGate->>SessionAuth: invoke authentication
SessionAuth->>Session: read auth_token / auth_id
SessionAuth-->>Server: Authentication success (ROLE_ADMIN)
Server-->>Vue: Serve SPA (protected)
Vue-->>Browser: Render protected view
end
sequenceDiagram
participant Vue as SubscriberDirectory (Client)
participant Server as SubscribersController (Symfony)
participant API as SubscribersClient (REST)
participant Normalizer as SubscriberCollectionNormalizer
Vue->>Server: GET /subscribers?after_id=&filters...
Server->>API: getSubscribers(filter, afterId, limit)
API-->>Server: SubscriberCollection
Server->>Normalizer: normalize(collection, prevId, afterId)
Normalizer-->>Server: { items, pagination }
Server-->>Vue: JSON { items, pagination }
Vue->>Vue: update UI, sync URL (history.replaceState)
User->>Vue: interact (search / filter / paginate)
Vue->>Server: repeat request with updated params
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (13)
assets/vue/components/base/BaseProgressBar.vue (1)
3-8: Add progressbar ARIA semantics.Consider exposing
role="progressbar"witharia-valuemin,aria-valuemax, andaria-valuenowso assistive tech can interpret the indicator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/base/BaseProgressBar.vue` around lines 3 - 8, Add proper ARIA semantics to the progress bar by giving the progress container (the element with class "progress" or the inner "progress__fill") role="progressbar" and binding aria-valuemin, aria-valuemax, and aria-valuenow to the component props; ensure the component exposes/uses props named value (current), min (default 0) and max (default 100) and bind :aria-valuemin="min", :aria-valuemax="max", :aria-valuenow="value" so assistive tech can interpret the indicator (keep visual behavior unchanged)..husky/pre-commit (1)
4-11: Prefer opt-out over fully disabling local checks.Commenting all checks removes fast local feedback. Since CI still enforces them, a lightweight pattern is to keep them on by default and allow explicit skip (
SKIP_LOCAL_QA=1) when needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.husky/pre-commit around lines 4 - 11, Restore the local QA commands (phpstan, phpmd, phpcs) instead of fully commenting them out and make them opt-out using an environment variable; wrap the existing commands (the php vendor/bin/phpstan, php vendor/bin/phpmd, and php vendor/bin/phpcs lines) in a conditional that skips execution when SKIP_LOCAL_QA=1 (for example: if [ "$SKIP_LOCAL_QA" != "1" ]; then <command> || exit 1; fi) so checks run by default but can be disabled explicitly.package.json (1)
9-9:file-loaderappears to be unused; consider removing it from dependencies.While Webpack 5 is in use,
file-loaderisn't explicitly configured in the webpack config—assets are handled via Webpack Encore'scopyFiles()instead. This dependency can likely be safely removed as dead weight, rather than needing migration to native asset modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 9, The package.json contains an unused dependency "file-loader"—remove the "file-loader" entry from package.json dependencies, run your package manager (npm install or yarn install) to update lockfiles, and verify there are no runtime references by searching for "file-loader" in the repo (e.g., any webpack/Encore config or imports). If CI or build scripts reference it, update them to rely on Encore's copyFiles()/Webpack 5 asset handling instead; then run the build locally to confirm nothing breaks.composer.json (1)
19-22: Pin the custom VCS package dependency to a commit hash instead of a branch reference.The
tatevikgr/rest-api-clientpackage uses the branch reference"dev-ISSUE-357"(composer.json, line 54), which can cause dependency drift across environments. Even thoughcomposer.lockis appropriately gitignored, pinning to a specific commit hash incomposer.jsonensures deterministic builds.Instead of
"dev-ISSUE-357", use a commit hash reference like:"tatevikgr/rest-api-client": "dev-ISSUE-357#commit-hash-here"Or verify the branch is stable and document the build process so
composer.lockis committed in CI environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composer.json` around lines 19 - 22, The dependency entry for "tatevikgr/rest-api-client" uses a branch ref ("dev-ISSUE-357") which can drift; update the version string in composer.json to pin to a specific commit by appending the commit hash (e.g. "dev-ISSUE-357#<commit-hash>") for deterministic installs, or alternatively replace the branch ref with a proper tag if one exists; locate the "tatevikgr/rest-api-client" requirement in composer.json and change its version value accordingly and run composer update to regenerate composer.lock in CI.assets/vue/components/dashboard/KpiGrid.vue (1)
27-29: Aligntrendvalues with the card contract.Line 28 uses
trend: 'neutral', whileassets/vue/components/dashboard/KpiCard.vuedocuments trend as"up" | "down"(Line 27). Please align these so variants don’t silently fall through styling/behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/dashboard/KpiGrid.vue` around lines 27 - 29, The KPI items in KpiGrid.vue set trend: 'neutral', which does not match the KpiCard.vue contract ("up" | "down"); update the trend values in the KPI object(s) (the trend property in the object with keys change/trend/icon) to use "up" or "down" instead of "neutral" — e.g., map zero/positive changes to "up" and negative changes (leading '-' in change like '-1.2%') to "down", or choose a default "up" if you prefer, so styling/behavior in KpiCard.vue receives only "up" or "down".assets/vue/RecentCampaignsCard.vue (1)
13-26: Unify this card’s data contract with the dashboard variant.This version hardcodes
rows, whileassets/vue/components/dashboard/RecentCampaignsCard.vueexpectsrowsvia props. Keeping both patterns will drift and complicate reuse.Refactor sketch
<script setup> import BaseCard from './components/base/BaseCard.vue' import CampaignsTable from './CampaignsTable.vue' -const rows = [ - { - id: 1, - name: 'Monthly Newsletter - June', - status: 'Sent', - date: '2024-06-01', - openRate: '24.5%', - clickRate: '3.2%', - }, - // ... -] +const props = defineProps({ + rows: { + type: Array, + default: () => [], + }, +}) </script>- <CampaignsTable :rows="rows" /> + <CampaignsTable :rows="props.rows" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/RecentCampaignsCard.vue` around lines 13 - 26, The RecentCampaignsCard.vue file currently hardcodes a local const rows which diverges from the dashboard variant; change RecentCampaignsCard.vue to accept rows as a prop (e.g., add a props declaration for rows with an Array type and a sensible default like []), remove the hardcoded const rows, and pass that prop into the internal CampaignsTable (or use the same prop name the dashboard variant uses) so both cards share the same data contract; update any references to the local rows variable to use the prop instead (look for the RecentCampaignsCard component, the rows const, and where CampaignsTable is used).assets/app.js (1)
1-1: Drop debugconsole.logfrom entrypoint before merge.This is useful locally, but noisy in production/devtools for all users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/app.js` at line 1, Remove the transient debug statement from the entrypoint: locate the console.log('app.js loaded') call in assets/app.js and delete it (or replace it with a conditional debug/log-level call behind an environment check such as process.env.NODE_ENV === 'development' or a logger.debug call) so the noisy message is not emitted in production/devtools.assets/vue/DashboardView.vue (1)
16-16: Consider wiringrowsfrom parent state.Right now this renders the card without data input, so it will rely on an empty fallback. Passing rows here will make dashboard behavior explicit and easier to swap to API data later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/DashboardView.vue` at line 16, RecentCampaignsCard is being rendered without its expected rows prop; wire the component to pass dashboard state (e.g., the parent data/computed property holding campaign rows such as recentCampaigns, recentCampaignRows, or similar) into the RecentCampaignsCard via a rows prop so the card receives explicit data instead of falling back to empty defaults—update the template where <RecentCampaignsCard /> is used to <RecentCampaignsCard :rows="yourRowsState" /> and ensure the referenced state (recentCampaigns/recentCampaignRows) is defined and populated in the parent component.assets/vue/SystemOverviewCard.vue (1)
1-50: Consolidate duplicateSystemOverviewCardimplementations.There are two separate
SystemOverviewCard.vuefiles (assets/vue/SystemOverviewCard.vueandassets/vue/components/dashboard/SystemOverviewCard.vue) with overlapping responsibility but different markup. This is a maintenance trap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/SystemOverviewCard.vue` around lines 1 - 50, There are duplicate SystemOverviewCard implementations; consolidate them by choosing one canonical component (the one that includes the props queuePercent and remaining and uses BaseCard, BaseBadge, BaseProgressBar, BaseButton) and merge any missing markup/props from the other implementation into it, then remove the other file and update any imports to point to the single SystemOverviewCard component; ensure the canonical component exports the same props (queuePercent, remaining) and preserves used child components (BaseCard, BaseBadge, BaseProgressBar, BaseButton) so every reference across the app continues to work.src/EventSubscriber/AuthGateSubscriber.php (1)
14-18: Consider retiring this temporary gate now that security is configured.This duplicates authentication decisions already handled by the firewall/authenticator and can drift over time. Consolidating auth flow in one place will reduce maintenance risk.
Also applies to: 33-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EventSubscriber/AuthGateSubscriber.php` around lines 14 - 18, AuthGateSubscriber's temporary auth logic is now duplicative; remove or disable the class so Symfony SecurityBundle handles auth centrally: locate the AuthGateSubscriber class and its onKernelRequest method (and any getSubscribedEvents/static subscription) and either delete the class or unregister it from DI/event subscriber registration, and remove any redirect-to-login logic and public-path checks (e.g., methods/properties handling publicPaths) so authentication decisions are solely handled by the firewall/authenticator.config/packages/security.yaml (1)
18-19: Tighten the public login matcher to an exact route.Line 18 uses
^/login, which also matches/login-anything. If only the login page should be public, anchor it to the exact path.Proposed fix
- - { path: ^/login, roles: PUBLIC_ACCESS } + - { path: ^/login$, roles: PUBLIC_ACCESS } - { path: ^/, roles: ROLE_ADMIN }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/packages/security.yaml` around lines 18 - 19, The public login path matcher currently uses the regex '^/login' which also matches '/login-anything'; update the matcher for the login route (the entry with path '^/login' and roles 'PUBLIC_ACCESS') to anchor it to the exact path (e.g., require the end-of-string anchor) so only '/login' is public while keeping the admin rule for the root ('^/') unchanged.src/Controller/DashboardController.php (1)
23-25: Remove dead commented auth logic.Line 23-Line 25 is stale commented code; dropping it keeps the controller clean and avoids confusion about where auth is enforced.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/DashboardController.php` around lines 23 - 25, Remove the stale commented authentication check inside DashboardController (the commented block that checks $request->getSession()->has('auth_token') and redirects to 'login'); delete those commented lines so the controller no longer contains dead/commented auth logic and rely on actual middleware/security layers instead.templates/base.html.twig (1)
19-19: Remove the emptyclassattribute on<main>.Line 19 has
class="", which adds markup noise without effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/base.html.twig` at line 19, Remove the redundant empty class attribute from the <main> element in templates/base.html.twig by deleting the class="" so the tag is simply <main>; update the <main> element usage wherever referenced to avoid leaving an empty attribute.
🤖 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/router/index.js`:
- Around line 3-5: The router currently created by createRouter has an empty
routes array so navigation targets like "/dashboard" won't resolve; add route
objects to the routes array (e.g., { path: '/', name: 'Home', component: Home },
{ path: '/dashboard', name: 'Dashboard', component: Dashboard }, etc.), import
the corresponding components (Dashboard, Home, NotFound) and include a catch-all
404 route as needed in the same file where router is defined (reference: router
/ createRouter / routes). Also ensure your main layout component (e.g.,
AppSidebar.vue or App.vue) includes the Vue Router outlet by adding a
<router-view /> placeholder where routed components should render so navigation
has a render target (reference: AppSidebar.vue / router-view).
In `@assets/vue/App.vue`:
- Around line 2-51: App.vue is missing a RouterView so routed pages never render
and the sidebar configuration is duplicated and inconsistent with
AppSidebar.vue; add a <RouterView /> in App.vue's template where main content
should render, remove the duplicated sections array from App.vue and instead
import the canonical sidebar configuration from AppSidebar.vue (or a new shared
module) so both components use the same data, and reconcile differences in the
sections data (ensure items like "Campaigns" have the same badge and icons such
as 'chart' vs 'chart-bar') — update references to the sections constant and keep
usage of SidebarNavSection and SidebarLogo unchanged.
In `@assets/vue/CampaignsTable.vue`:
- Around line 23-27: Guard the use of row.status before calling toLowerCase in
CampaignsTable.vue: update the class binding that uses
`row.status.toLowerCase()` to handle null/undefined/non-string values (for
example by checking typeof or using a safe default like an empty string) so it
never calls toLowerCase on a non-string; ensure the interpolation `{{ row.status
}}` stays unchanged but the computed/class expression that builds
`campaigns-table__status--${...}` uses a safe expression (e.g., `typeof
row.status === 'string' ? row.status.toLowerCase() : 'unknown'`) to prevent
runtime errors during render.
In `@assets/vue/components/base/BaseCard.vue`:
- Line 15: variantClass is currently a static string and won't update when
props.variant changes; change its definition to a Vue computed that returns
`base-card--${props.variant}` (use the computed() API) so the class reacts to
prop updates, ensure computed is imported from 'vue' and that any template or
class binding uses the new computed ref (variantClass) instead of the static
variable in BaseCard.vue.
In `@assets/vue/components/base/BaseIcon.vue`:
- Around line 14-40: The icon registry contains trailing non-SVG markup that
will corrupt injected HTML: remove the stray "<span class=\"font-medium
text-sm\">" suffixes from the users, plane, grid, list, and layout entries and
remove the stray "</button>" from the edit entry in the BaseIcon.vue icons
object so each value ends exactly with the closing </svg>; locate the icons by
their keys (users, plane, grid, list, layout, edit) and update their string
values to contain only valid SVG markup.
In `@assets/vue/components/base/BaseProgressBar.vue`:
- Around line 6-7: Clamp the incoming progress prop before using it in the
template: create a computed (or method) such as clampedValue that bounds the
component prop value to 0..100 and use clampedValue in the style binding instead
of raw value; update both places where ":style=\"{ width: value + '%' }\""
appears (the inline width on the progress bar and the alternate instance around
lines 12-17) to reference clampedValue so out-of-range numbers cannot produce
invalid widths or visual glitches.
In `@assets/vue/components/charts/LineChart.vue`:
- Around line 62-75: The code that builds SVG points (using props.series,
allValues, max, min, range, pointCount, normalized, x, y) assumes every
series.data item is a finite number and can produce NaN; fix by normalizing each
series.data to a cleaned numeric array (map values through Number(value) or
parseFloat and filter with Number.isFinite), compute allValues from those
cleaned arrays (guarding for empty by returning an empty array from the function
or an empty string for that series), protect range from zero/NaN by using a
fallback (e.g., range = max === min || !isFinite(range) ? 1 : range), and ensure
pointCount is computed from the cleaned data (and guard pointCount <= 1) so x
and normalized computations never produce NaN; return sensible empty outputs for
series with no valid points.
In `@assets/vue/components/dashboard/PerformanceChartCard.vue`:
- Around line 9-17: The PerformanceChartCard component is missing props and the
real chart render: add a defineProps declaration to accept the passed-in props
(labels and series) and update the template to render the LineChart instead of
the placeholder; specifically, in the PerformanceChartCard component that
currently imports BaseCard, declare defineProps({ labels: Array, series: Array
}) (or appropriate types), import/register the LineChart used by the root-level
assets/vue/PerformanceChartCard.vue, and replace the <div
class="pc-chart-placeholder"> block with the LineChart element bound to
:labels="labels" and :series="series" so DashboardView.vue’s bindings are
consumed.
In `@assets/vue/components/dashboard/SystemOverviewCard.vue`:
- Around line 13-14: Clamp the queuePercent value to the 0–100 range before
binding it to the style so the progress bar cannot render with negative or >100
widths; update the usage of queuePercent in SystemOverviewCard.vue (the
computed/prop/variable named queuePercent used in the element with class
"sys-progress-fill") to compute a clampedQueuePercent = Math.min(100,
Math.max(0, queuePercent)) and use that clamped value in the :style width
expression.
In `@assets/vue/DashboardView.vue`:
- Line 24: The import for KpiGrid in DashboardView.vue uses an incorrect
absolute path; update the import statement that references KpiGrid so it uses
the same relative path style as the other components (e.g., change import
KpiGrid from '/components/dashboard/KpiGrid.vue' to the relative
'./components/dashboard/KpiGrid.vue' pattern used elsewhere) so the module
resolver can find the KpiGrid component.
In `@assets/vue/SidebarLogo.vue`:
- Around line 4-8: The img tag in SidebarLogo.vue uses a generic alt="Logo";
update the template to bind the alt attribute to a meaningful value (e.g., a new
or existing prop like altText or the app name) or set alt="" when adjacent text
already conveys the branding; change the <img ... :src="logo" alt="Logo" /> to
use a bound attribute (e.g., :alt="altText || 'YourAppName'") and add a
prop/interface for altText in the SidebarLogo component so consumers can provide
accessible, descriptive alt text.
In `@src/Controller/AuthController.php`:
- Line 53: The login flow in AuthController.php is inconsistent: the post-login
branch redirects to redirectToRoute('dashboard') while the already-authenticated
branch sends users to redirectToRoute('empty_start_page'); update the
already-authenticated branch to use redirectToRoute('dashboard') so both
branches use the same target. Locate the method handling the /login route (e.g.,
login() in AuthController) and replace the redirectToRoute('empty_start_page')
call with redirectToRoute('dashboard') to unify the authenticated redirect
behavior.
In `@src/Controller/DashboardController.php`:
- Around line 15-18: DashboardController currently assigns $this->apiClient in
__construct without a declared property; either declare it using PHP 8.1
constructor property promotion (e.g., change __construct(AuthClient $apiClient)
to __construct(private AuthClient $apiClient)) or remove the parameter and
assignment if DashboardController does not use AuthClient; reference the
DashboardController class, its __construct method and the AuthClient type
(compare to AuthController's private AuthClient $apiClient) to locate and apply
the change.
In `@src/Security/SessionAuthenticator.php`:
- Around line 39-49: The code currently grants ROLE_ADMIN whenever
$session->has('auth_token'); instead, retrieve the token value
($session->get('auth_token')) and validate it (expiry, signature or remote
introspection) before creating the UserBadge and SelfValidatingPassport. Replace
the unconditional UserBadge creation in the SessionAuthenticator authenticate
method with a call to a token validation helper or injected
TokenIntrospector/TokenValidator service (e.g., validateAuthToken or
introspectAuthToken) and throw AuthenticationException when validation fails;
derive roles from the token claims instead of always assigning ['ROLE_ADMIN'] so
UserBadge('session-user', ...) uses the validated roles. Ensure the authenticate
method returns the passport only for a validated token.
- Around line 25-33: The supports(Request $request) method currently only skips
/login and should also skip public/debug asset routes; update the supports()
function to return false when $request->getPathInfo() starts with any of the
known public/debug prefixes (e.g. '/build/', '/_profiler', '/_wdt') or other
static endpoints you need (favicon/robots etc.), either by adding individual
str_starts_with checks or by iterating an array of prefixes and returning false
if any match; keep the existing /login skip and otherwise return true.
In `@templates/auth/login.html.twig`:
- Line 42: Replace the no-op anchor links that use href="#" in
templates/auth/login.html.twig (notably the "Forgot password?" anchor at line
with class "small text-decoration-none" and the group at lines 63-66) with real
destinations or non-link elements: update the anchors to point to the correct
route names/URLs (e.g., password reset, help, or terms) using your template
route helper, or convert them to <button> or <span> with appropriate
classes/role and click handlers if they trigger JS actions; ensure the unique
anchor instances (the "Forgot password?" anchor and the anchors in the 63-66
block) no longer use href="#" to avoid jump-to-top behavior.
- Around line 31-58: The login form currently posts credentials without CSRF
protection; add a hidden input field inside the <form> (alongside the username
and password inputs) that contains the CSRF token value (use your framework/Twig
CSRF helper, e.g. the token for the authentication action) and ensure the
server-side login handler validates that token (check the expected request
parameter name and use the same token id/intent as generated in the template)
before accepting credentials; update the login handler to return an error on
invalid/missing tokens.
- Around line 10-13: The template uses Bootstrap 5 utility classes that don’t
exist in the loaded Bootstrap 4.5.2; update the class names in
templates/auth/login.html.twig (e.g., the <img> tag using "me-2 mr-2" and the
<h1> using "fs-4 fw-bold") and the other referenced locations (lines referenced
in the review) to Bootstrap 4 equivalents: replace me-* with mr-*, ms-* with
ml-*, replace fw-bold / fw-semibold with Bootstrap 4 font-weight classes (e.g.,
font-weight-bold or font-weight-normal) and replace fs-* with appropriate
Bootstrap 4 utilities (use heading/display classes like display-4 or standard h*
tags or add a small custom CSS rule if an exact size is required); apply these
same substitutions for the other occurrences noted (lines 22-23, 49-50, 63-67).
In `@templates/base.html.twig`:
- Around line 14-16: The template double-loads the Webpack Encore app bundle:
the javascripts block already calls encore_entry_script_tags('app'), but the
template later also statically includes build/runtime.js and build/app.js;
remove (or conditionally skip) the manual script tags so the app is only
initialized once — locate the javascripts block and the static script tags
referencing build/runtime.js and build/app.js and delete the redundant static
includes (or replace them with a single call to
encore_entry_script_tags('app')).
In `@templates/dashboard/index.html.twig`:
- Line 1: The header comment in the template incorrectly references
templates/security/login.html.twig; update the top-of-file comment in
templates/dashboard/index.html.twig (the existing comment string "{#
templates/security/login.html.twig #}") to either the correct path "{#
templates/dashboard/index.html.twig #}" or remove the misleading header entirely
so it accurately reflects this template's location.
---
Nitpick comments:
In @.husky/pre-commit:
- Around line 4-11: Restore the local QA commands (phpstan, phpmd, phpcs)
instead of fully commenting them out and make them opt-out using an environment
variable; wrap the existing commands (the php vendor/bin/phpstan, php
vendor/bin/phpmd, and php vendor/bin/phpcs lines) in a conditional that skips
execution when SKIP_LOCAL_QA=1 (for example: if [ "$SKIP_LOCAL_QA" != "1" ];
then <command> || exit 1; fi) so checks run by default but can be disabled
explicitly.
In `@assets/app.js`:
- Line 1: Remove the transient debug statement from the entrypoint: locate the
console.log('app.js loaded') call in assets/app.js and delete it (or replace it
with a conditional debug/log-level call behind an environment check such as
process.env.NODE_ENV === 'development' or a logger.debug call) so the noisy
message is not emitted in production/devtools.
In `@assets/vue/components/base/BaseProgressBar.vue`:
- Around line 3-8: Add proper ARIA semantics to the progress bar by giving the
progress container (the element with class "progress" or the inner
"progress__fill") role="progressbar" and binding aria-valuemin, aria-valuemax,
and aria-valuenow to the component props; ensure the component exposes/uses
props named value (current), min (default 0) and max (default 100) and bind
:aria-valuemin="min", :aria-valuemax="max", :aria-valuenow="value" so assistive
tech can interpret the indicator (keep visual behavior unchanged).
In `@assets/vue/components/dashboard/KpiGrid.vue`:
- Around line 27-29: The KPI items in KpiGrid.vue set trend: 'neutral', which
does not match the KpiCard.vue contract ("up" | "down"); update the trend values
in the KPI object(s) (the trend property in the object with keys
change/trend/icon) to use "up" or "down" instead of "neutral" — e.g., map
zero/positive changes to "up" and negative changes (leading '-' in change like
'-1.2%') to "down", or choose a default "up" if you prefer, so styling/behavior
in KpiCard.vue receives only "up" or "down".
In `@assets/vue/DashboardView.vue`:
- Line 16: RecentCampaignsCard is being rendered without its expected rows prop;
wire the component to pass dashboard state (e.g., the parent data/computed
property holding campaign rows such as recentCampaigns, recentCampaignRows, or
similar) into the RecentCampaignsCard via a rows prop so the card receives
explicit data instead of falling back to empty defaults—update the template
where <RecentCampaignsCard /> is used to <RecentCampaignsCard
:rows="yourRowsState" /> and ensure the referenced state
(recentCampaigns/recentCampaignRows) is defined and populated in the parent
component.
In `@assets/vue/RecentCampaignsCard.vue`:
- Around line 13-26: The RecentCampaignsCard.vue file currently hardcodes a
local const rows which diverges from the dashboard variant; change
RecentCampaignsCard.vue to accept rows as a prop (e.g., add a props declaration
for rows with an Array type and a sensible default like []), remove the
hardcoded const rows, and pass that prop into the internal CampaignsTable (or
use the same prop name the dashboard variant uses) so both cards share the same
data contract; update any references to the local rows variable to use the prop
instead (look for the RecentCampaignsCard component, the rows const, and where
CampaignsTable is used).
In `@assets/vue/SystemOverviewCard.vue`:
- Around line 1-50: There are duplicate SystemOverviewCard implementations;
consolidate them by choosing one canonical component (the one that includes the
props queuePercent and remaining and uses BaseCard, BaseBadge, BaseProgressBar,
BaseButton) and merge any missing markup/props from the other implementation
into it, then remove the other file and update any imports to point to the
single SystemOverviewCard component; ensure the canonical component exports the
same props (queuePercent, remaining) and preserves used child components
(BaseCard, BaseBadge, BaseProgressBar, BaseButton) so every reference across the
app continues to work.
In `@composer.json`:
- Around line 19-22: The dependency entry for "tatevikgr/rest-api-client" uses a
branch ref ("dev-ISSUE-357") which can drift; update the version string in
composer.json to pin to a specific commit by appending the commit hash (e.g.
"dev-ISSUE-357#<commit-hash>") for deterministic installs, or alternatively
replace the branch ref with a proper tag if one exists; locate the
"tatevikgr/rest-api-client" requirement in composer.json and change its version
value accordingly and run composer update to regenerate composer.lock in CI.
In `@config/packages/security.yaml`:
- Around line 18-19: The public login path matcher currently uses the regex
'^/login' which also matches '/login-anything'; update the matcher for the login
route (the entry with path '^/login' and roles 'PUBLIC_ACCESS') to anchor it to
the exact path (e.g., require the end-of-string anchor) so only '/login' is
public while keeping the admin rule for the root ('^/') unchanged.
In `@package.json`:
- Line 9: The package.json contains an unused dependency "file-loader"—remove
the "file-loader" entry from package.json dependencies, run your package manager
(npm install or yarn install) to update lockfiles, and verify there are no
runtime references by searching for "file-loader" in the repo (e.g., any
webpack/Encore config or imports). If CI or build scripts reference it, update
them to rely on Encore's copyFiles()/Webpack 5 asset handling instead; then run
the build locally to confirm nothing breaks.
In `@src/Controller/DashboardController.php`:
- Around line 23-25: Remove the stale commented authentication check inside
DashboardController (the commented block that checks
$request->getSession()->has('auth_token') and redirects to 'login'); delete
those commented lines so the controller no longer contains dead/commented auth
logic and rely on actual middleware/security layers instead.
In `@src/EventSubscriber/AuthGateSubscriber.php`:
- Around line 14-18: AuthGateSubscriber's temporary auth logic is now
duplicative; remove or disable the class so Symfony SecurityBundle handles auth
centrally: locate the AuthGateSubscriber class and its onKernelRequest method
(and any getSubscribedEvents/static subscription) and either delete the class or
unregister it from DI/event subscriber registration, and remove any
redirect-to-login logic and public-path checks (e.g., methods/properties
handling publicPaths) so authentication decisions are solely handled by the
firewall/authenticator.
In `@templates/base.html.twig`:
- Line 19: Remove the redundant empty class attribute from the <main> element in
templates/base.html.twig by deleting the class="" so the tag is simply <main>;
update the <main> element usage wherever referenced to avoid leaving an empty
attribute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dba409ed-a83e-4043-94f8-96700fdcb1e8
⛔ Files ignored due to path filters (3)
assets/images/logo-48.pngis excluded by!**/*.pngassets/images/logo.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (41)
.husky/pre-commitassets/app.jsassets/router/index.jsassets/vue/App.vueassets/vue/AppSidebar.vueassets/vue/BaseCard.vueassets/vue/CampaignsTable.vueassets/vue/DashboardView.vueassets/vue/PerformanceChartCard.vueassets/vue/RecentCampaignsCard.vueassets/vue/SidebarLogo.vueassets/vue/SidebarNavItem.vueassets/vue/SidebarNavSection.vueassets/vue/SystemOverviewCard.vueassets/vue/components/base/BaseBadge.vueassets/vue/components/base/BaseButton.vueassets/vue/components/base/BaseCard.vueassets/vue/components/base/BaseIcon.vueassets/vue/components/base/BaseProgressBar.vueassets/vue/components/charts/LineChart.vueassets/vue/components/dashboard/KpiCard.vueassets/vue/components/dashboard/KpiGrid.vueassets/vue/components/dashboard/PerformanceChartCard.vueassets/vue/components/dashboard/RecentCampaignsCard.vueassets/vue/components/dashboard/SystemOverviewCard.vueassets/vue/layouts/DashboardLayout.vuecomposer.jsonconfig/packages/framework.yamlconfig/packages/security.yamlconfig/services.ymlpackage.jsonsrc/Controller/AuthController.phpsrc/Controller/DashboardController.phpsrc/EventSubscriber/AuthGateSubscriber.phpsrc/Security/SessionAuthenticator.phpsrc/Service/ApiClient.phptemplates/auth/login.html.twigtemplates/base.html.twigtemplates/dashboard/index.html.twigtests/Unit/Service/ApiClientTest.phpwebpack.config.js
💤 Files with no reviewable changes (2)
- tests/Unit/Service/ApiClientTest.php
- src/Service/ApiClient.php
Comment on lines
+14
to
+16
| {% block javascripts %} | ||
| {{ encore_entry_script_tags('app') }} | ||
| {% endblock %} |
There was a problem hiding this comment.
Avoid double-loading the Encore app bundle.
Line 15 already injects the JS entry, but build/runtime.js and build/app.js are still loaded at Line 26 and Line 27. That can initialize the app twice.
Proposed fix
{% block javascripts %}
{{ encore_entry_script_tags('app') }}
{% endblock %}
@@
-<script src="{{ asset('build/runtime.js') }}"></script>
-<script src="{{ asset('build/app.js') }}"></script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/base.html.twig` around lines 14 - 16, The template double-loads the
Webpack Encore app bundle: the javascripts block already calls
encore_entry_script_tags('app'), but the template later also statically includes
build/runtime.js and build/app.js; remove (or conditionally skip) the manual
script tags so the app is only initialized once — locate the javascripts block
and the static script tags referencing build/runtime.js and build/app.js and
delete the redundant static includes (or replace them with a single call to
encore_entry_script_tags('app')).
* Css * Css + bootstrap * Sidebar * grid * sticky * routes * Update rest-client and core * Redirect to home if logged in * Exclude api from firewall * ApiSessionListener * SubscribersController * testing * Use tailwind * Subscribers * Subscribers pass data * Color ext-wf1 * Align icon with text * Mobile * Filter subscribers * Filter subscribers * Fix: created * ref: SubscribersController * Fix: loading for SPA * Fix: test * Fix: style * Fix: tests * spa.html.twig * subscriberFilters * find subscriber by email * find subscriber by email, foreign key and unique id * download csv * Export CSV * Apache config * Export filtered * Sort * Fix: logout * Fix: autowiring * Add: auth user data retrieval * remove example page * Logout button * Add subscriber modal * Add details to subscriber modal * install js client * Fix: js client * Fix: filter * ImportSubscribers * coderabbit config * After review 0 * After review 1 --------- Co-authored-by: Tatevik <tatevikg1@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (3)
templates/auth/login.html.twig (2)
38-38:⚠️ Potential issue | 🟡 Minor
href="#"placeholder for "Forgot password?" — still needs a real destination.This was noted in a previous review. Either wire it to an actual password reset route or remove the link if the feature doesn't exist yet.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/auth/login.html.twig` at line 38, The "Forgot password?" anchor uses a placeholder href="#" and must be wired to a real route or removed; update the anchor in templates/auth/login.html.twig so its href calls the correct password-reset route (e.g., use Twig's path('password_reset') or the actual route name used by your app) or remove the <a> entirely if password reset isn't implemented yet, and ensure the class/text remain unchanged.
29-52:⚠️ Potential issue | 🟠 MajorCSRF token still missing from login form.
This was flagged in a previous review and remains unaddressed. The form submits credentials without CSRF protection, which is a security concern.
🔒 Add CSRF token
<form method="post"> + <input type="hidden" name="_csrf_token" value="{{ csrf_token('authenticate') }}"> <div class="mb-4">Make sure the server-side login handler validates this token.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/auth/login.html.twig` around lines 29 - 52, The login form in login.html.twig is missing a CSRF token input; add the framework's CSRF hidden field inside the <form> (e.g., insert the Twig CSRF token helper or a hidden input named like csrf_token) and ensure the server-side login handler (the route/controller that processes the form submission) validates this token before authenticating the user; reference the form element in the template and the login handler method to implement token generation, insertion, and validation.templates/base.html.twig (1)
12-14:⚠️ Potential issue | 🟠 MajorDouble-loading the app bundle — still needs fixing.
The
encore_entry_script_tags('app')at Line 13 already injectsruntime.jsandapp.js. The manual script tags at Lines 21-22 load them again, which will initialize the Vue app twice.Remove the static script tags at the end of the body.
Proposed fix
</main> -<script src="{{ asset('build/runtime.js') }}"></script> -<script src="{{ asset('build/app.js') }}"></script> </body> </html>Also applies to: 21-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/base.html.twig` around lines 12 - 14, The template currently double-loads the app bundle: keep the Twig block that calls encore_entry_script_tags('app') inside the javascripts block and remove the duplicate manual <script> tags that statically include runtime.js and app.js at the end of the body; locate the static tags near the bottom of templates/base.html.twig and delete them so the Vue app is only initialized via encore_entry_script_tags('app').
🧹 Nitpick comments (27)
apache/web-frontend.conf (2)
21-25: Hardcoded PHP-FPM socket version.The socket path
/run/php/php8.1-fpm.socklocks this to PHP 8.1. If someone runs a different PHP version, requests will fail silently (proxy error).♻️ Consider a more flexible approach
<IfModule proxy_fcgi_module> <FilesMatch \.php$> - SetHandler "proxy:unix:/run/php/php8.1-fpm.sock|fcgi://localhost" + # Adjust socket path to match your PHP-FPM version + SetHandler "proxy:unix:/run/php/php-fpm.sock|fcgi://localhost" </FilesMatch> </IfModule>Alternatively, add a comment noting the version requirement, or use an environment variable via
Define.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apache/web-frontend.conf` around lines 21 - 25, The SetHandler in the FilesMatch block currently hardcodes the PHP-FPM socket path "proxy:unix:/run/php/php8.1-fpm.sock|fcgi://localhost", which will break if PHP is upgraded; update the FilesMatch/SetHandler usage to reference a configurable Define or environment-provided variable (e.g., PHP_FPM_SOCK) instead of the literal path, or document the required PHP version and instruct maintainers to create a stable symlink (/run/php/php-fpm.sock) that points to the active version; adjust the SetHandler reference in the FilesMatch block accordingly so the socket path is not tied to "php8.1" directly.
1-2: HTTP-only VirtualHost — confirm this is intentional for local dev.This config uses port 80 without TLS. Fine for local development (
frontend.phplist.local), but ensure there's a separate HTTPS config or reverse proxy for production deployments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apache/web-frontend.conf` around lines 1 - 2, This VirtualHost is configured for HTTP only (VirtualHost *:80 with ServerName frontend.phplist.local); if this is only for local dev, add a clear comment stating that and restrict access (e.g., bind to localhost or local-only network), otherwise create a matching HTTPS VirtualHost (VirtualHost *:443) with TLS settings (certificate, SSLEngine on) or ensure a production reverse proxy handles TLS so production is not served over plain HTTP.assets/vue/components/dashboard/CampaignsTable.vue (1)
47-53: Unusedpropsvariable.
definePropsreturns the props object but it's assigned topropswhich is never referenced. ThestatusClassfunction receivesstatusas a parameter instead of reading fromprops.rows.♻️ Simplified props declaration
<script setup> -const props = defineProps({ +defineProps({ rows: { type: Array, default: () => [], }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/dashboard/CampaignsTable.vue` around lines 47 - 53, The code assigns defineProps(...) to a never-used variable "props"; remove the unused variable by replacing "const props = defineProps({ ... })" with a direct call "defineProps({ ... })" so props are declared for the component without creating an unused identifier, and verify that statusClass continues to accept a status parameter (no change needed) rather than trying to read props.rows.assets/vue/components/sidebar/SidebarLogo.vue (1)
4-9: Conflicting height classes on the image element.The
<img>has bothh-full(line 5) andh-auto(also line 5) applied. These conflict —h-autowill overrideh-fullbut the intent is unclear. If you want the image to fill the container, droph-auto; if you want natural aspect ratio, droph-full.♻️ Suggested fix (assuming fill container)
<img - class="w-full h-full max-w-full h-auto block" + class="w-full h-full block" :src="logo" alt="Logo" style="object-fit: contain;" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/sidebar/SidebarLogo.vue` around lines 4 - 9, The img in SidebarLogo.vue has conflicting Tailwind height classes on the same element (class="... h-full h-auto ..."); decide the intended behavior and remove the other class: if you want the logo to fill its container keep h-full and remove h-auto, or if you want natural aspect ratio keep h-auto and remove h-full; update the class attribute on the <img> in the SidebarLogo component accordingly.package.json (1)
24-24: Consider pinning vue-router to a more specific version.Using
"vue-router": "4"lets any4.x.xrelease through, which can include unexpected updates. Vue Router's minor versions have introduced API changes, so tighter pinning is safer.♻️ Suggested fix
- "vue-router": "4" + "vue-router": "~4.6.4"The latest 4.x release is 4.6.4. The tilde (~) locks to patch updates only, preventing accidental breaking changes in minor releases. If you prefer less strict versioning,
"^4.6.4"works too, but~is more predictable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 24, Update the package.json dependency entry for "vue-router" to pin a more specific version to avoid pulling unintended minor releases; locate the "vue-router" entry in package.json and change its version string from "4" to a fixed or more conservative range such as "~4.6.4" (or "^4.6.4" if you prefer less strictness) so only safe patch updates are allowed.assets/vue/components/dashboard/RecentCampaignsCard.vue (1)
20-51: Mock data baked into component defaults — consider externalizing.The
rowsprop defaults to hardcoded sample campaigns with 2023 dates. SinceDashboardView.vue(lines 18-22) uses this component without passing any props, users will always see this stale mock data. The TODO on line 21 acknowledges this.A couple of options to clean this up:
- Fetch real data from the backend API in
DashboardViewand pass it down- Or at minimum, default to an empty array and show the table's empty state until real data arrives
Not blocking, but worth tracking as a follow-up.
Want me to open an issue to track wiring this up to fetch campaigns from the backend?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/dashboard/RecentCampaignsCard.vue` around lines 20 - 51, Replace the baked-in mock campaign data in the RecentCampaignsCard.vue props so the component no longer defaults to stale samples: change the defineProps({ rows: { ... default: (...) }}) to default to an empty array (or remove the default) and ensure the component (RecentCampaignsCard) handles an empty rows array via its empty state; then wire DashboardView to fetch real campaign data from the backend and pass it into the rows prop (or open an issue to track adding a fetch in DashboardView if you prefer to defer implementation).assets/vue/components/subscribers/SubscriberFilters.vue (1)
27-35: Clicking active filter sets it tonull— is that intentional UX?When clicking the currently active filter,
activeFilterbecomesnull(line 29). This means no filter button is highlighted, andnullis emitted to the parent.If
nullshould behave differently from'all', that's fine. But if they're meant to be equivalent, consider keeping'all'selected instead of allowing a "no selection" state that might confuse users:const toggleFilter = (filterId) => { // Option: always keep 'all' as fallback instead of null activeFilter.value = activeFilter.value === filterId ? 'all' : filterId emit('filter-change', activeFilter.value) }Just a UX thought — feel free to disregard if the null state is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/subscribers/SubscriberFilters.vue` around lines 27 - 35, The current toggleFilter function sets activeFilter.value to null when the user clicks the already-active filter, creating a "no selection" state; change toggleFilter so that when activeFilter.value === filterId it falls back to the default 'all' (i.e., activeFilter.value = 'all') instead of null, and continue to emit('filter-change', activeFilter.value) so the parent always receives a valid filter value; update the logic in the toggleFilter function and any usages relying on null to expect 'all' instead.composer.json (1)
50-54: Usingdev-*branches for dependencies — be mindful of stability.Both
phplist/core: dev-devandtatevikgr/rest-api-client: dev-mainpoint to moving targets. This is fine during active development, but consider:
- Pinning to specific commits or tags before release
- Documenting which commit hashes are known to work
Just a heads-up for when things stabilize.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composer.json` around lines 50 - 54, The composer.json currently depends on moving branches "phplist/core": "dev-dev" and "tatevikgr/rest-api-client": "dev-main"; replace those unstable refs with either a specific tagged version (e.g., "1.2.3") or a commit-pinned ref (e.g., "dev-main#<commit-hash>") for production releases, update the dependency entries for phplist/core and tatevikgr/rest-api-client accordingly, regenerate and commit composer.lock, and document in the repo (README or RELEASE notes) the chosen commit hashes or tags that are known to work.assets/vue/components/sidebar/SidebarNavSection.vue (1)
6-8: Use a stable unique key for nav items.Using
item.labelas the key is fragile if labels repeat. Preferitem.route(or an explicitid) to avoid patching glitches.Suggested tweak
- <SidebarNavItem - v-for="item in items" - :key="item.label" - :item="item" - /> + <SidebarNavItem + v-for="item in items" + :key="item.route" + :item="item" + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/sidebar/SidebarNavSection.vue` around lines 6 - 8, The v-for in SidebarNavSection.vue uses a fragile key (:key="item.label"); change the key to a stable unique identifier such as :key="item.route" or an explicit :key="item.id" (ensure each item in items has that property) so the list rendering for items remains stable and avoids patching glitches; update any data construction that builds items to include a unique id if necessary.assets/vue/components/subscribers/SubscriberTable.vue (2)
130-139: Dead code:formatDateis never called.This helper function is defined but not used anywhere in the template. The dates are rendered directly via
{{ subscriber.createdAt }}without formatting.Either remove
formatDateif it's not needed, or wire it up in the template to format the dates:- {{ subscriber.createdAt }} + {{ formatDate(subscriber.createdAt) }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/subscribers/SubscriberTable.vue` around lines 130 - 139, The helper function formatDate is dead code because the template renders raw dates with {{ subscriber.createdAt }}; either remove formatDate or wire it into the template so dates are formatted. To fix, either delete the unused formatDate function, or update the template where subscriber.createdAt is displayed to call the formatter (e.g., use the formatDate helper or map it via a computed/ method) so createdAt is passed to formatDate (and optionally set isIso) before rendering; ensure any import/registration needed for formatDate in the component options is added if you choose to keep it.
112-112: Unused import.The
injectimport from Vue isn't used anywhere in this component.🧹 Proposed fix
-import { inject } from 'vue'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/subscribers/SubscriberTable.vue` at line 112, The file imports Vue's inject but doesn't use it; remove the unused import to clean up SubscriberTable.vue by deleting "inject" from the import statement (or remove the entire import line if nothing else is imported), ensuring no references to inject remain in the component (check for symbols like inject or provide/inject usage) before committing.tests/Integration/Auth/LoginTest.php (2)
23-26: Inconsistent connection timeouts between tests.
testLoginPageFormFieldsAreVisibleuses 10000ms whiletestLoginFormSubmissionuses 20000ms. If the longer timeout was needed for reliability, consider using the same value in both tests to avoid flaky runs.Also applies to: 43-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Integration/Auth/LoginTest.php` around lines 23 - 26, Both tests use inconsistent Panther client connection timeouts; update the createPantherClient calls in LoginTest (used in testLoginPageFormFieldsAreVisible and testLoginFormSubmission) so they use the same timeout value (choose the longer 20000ms for reliability) or define a shared constant (e.g., DEFAULT_CONNECTION_TIMEOUT_MS) and use it in both places to avoid flaky runs.
55-55: Brittle selector: Tailwind utility classes.Using
.bg-red-50.border-red-200.text-red-600as a selector is fragile—any styling tweak will break this test. Consider adding a dedicated test ID or semantic class to the error alert element.In the template, add a test hook:
<div class="bg-red-50 border-red-200 text-red-600 ..." data-testid="login-error">Then in the test:
- $this->assertSelectorExists('.bg-red-50.border-red-200.text-red-600'); + $this->assertSelectorExists('[data-testid="login-error"]');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Integration/Auth/LoginTest.php` at line 55, The test uses a brittle Tailwind selector in LoginTest.php (the assertSelectorExists call with '.bg-red-50.border-red-200.text-red-600'); change the app template to add a stable test hook (e.g., data-testid="login-error" on the error alert div) and update the test to assertSelectorExists('[data-testid="login-error"]') instead of the utility-class selector so styling changes won't break the test.assets/vue/components/subscribers/SubscriberModal.vue (1)
202-204: Consider resetting state when modal closes.When the modal closes,
subscriber,formData, anderrorretain their previous values. If the user reopens the modal quickly (before the watcher triggers fetch), they might briefly see stale data.🧹 Proposed enhancement
const close = () => { + subscriber.value = null + error.value = null emit('close') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/subscribers/SubscriberModal.vue` around lines 202 - 204, The close() handler currently only emits 'close' but leaves local state (subscriber, formData, error) intact, causing stale values to flash if the modal is reopened; update the close function to reset subscriber to its initial empty value, reset formData to the initial form shape, and clear error before emitting 'close' (or emit after resetting) so the modal always starts with a clean state; reference the close() function and the subscriber, formData, and error reactive/state variables when making this change.tailwind.config.js (2)
6-8: Redundant content glob patterns.
./assets/vue/**/*.vue(Line 7) already matches everything that./assets/vue/*.vue(Line 6) and./assets/vue/**/**/*.vue(Line 8) would match. You can simplify to just the double-star pattern.Simplified content array
content: [ "./templates/**/*.twig", "./themes/**/**/*.twig", - "./assets/vue/*.vue", "./assets/vue/**/*.vue", - "./assets/vue/**/**/*.vue", ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tailwind.config.js` around lines 6 - 8, The content globs in tailwind.config.js are redundant: remove the single-star ("./assets/vue/*.vue") and the triple-star ("./assets/vue/**/**/*.vue") entries and keep the recursive double-star pattern ("./assets/vue/**/*.vue") only; update the exported content array so it contains the simplified pattern to avoid duplicate scanning and potential maintenance confusion.
10-17: Custom breakpoints replace Tailwind defaults.Defining
screensdirectly intheme(notextend) removes default breakpoints like2xl. If that's intentional, all good. If you need2xl(1536px) later, move this intoextend.screensinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tailwind.config.js` around lines 10 - 17, You replaced Tailwind's default breakpoints by declaring theme.screens directly; if you want to keep the defaults (including 2xl at 1536px) move your custom screens into theme.extend.screens instead of theme.screens so Tailwind merges them rather than overwriting—update the screens definition used in the tailwind.config.js (the theme and screens keys) to theme.extend.screens to preserve built-in breakpoints.templates/base.html.twig (1)
8-8: Potential duplicate CSS loading.Line 8 directly links
build/app.csswhile Line 10 usesencore_entry_link_tags('styles'). If both reference the same stylesheet, you're loading it twice. Consider removing the directasset()link if Encore handles it.Also applies to: 10-10
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/base.html.twig` at line 8, The template is potentially loading the same stylesheet twice: remove the direct <link rel="stylesheet" href="{{ asset('build/app.css') }}"> if your Webpack Encore entry uses encore_entry_link_tags('styles'); inspect the Encore entry named 'styles' (encore_entry_link_tags('styles')) and, if it outputs build/app.css, delete the asset() link to avoid duplicate CSS; if the Encore entry does not include that file, keep the asset() link or adjust the Encore configuration so the stylesheet is emitted via the 'styles' entry.assets/vue/components/subscribers/ImportResult.vue (1)
71-79: Redundantdefaultwithrequired: true.If
importResultis required, the default value is never used. Consider removing eitherrequired: true(if a default makes sense) or thedefaultfunction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/subscribers/ImportResult.vue` around lines 71 - 79, The importResult prop currently declares required: true while also providing a default value, which is redundant; decide whether this component should accept a missing prop or enforce it—either remove the default function from the importResult prop to keep required: true, or remove required: true and keep the default object ({ imported: 0, skipped: 0, errors: []}) so the component can operate without the parent passing importResult; update the importResult prop declaration accordingly in ImportResult.vue.src/Controller/AuthController.php (2)
53-57: Redundant exception catch.
GuzzleExceptionextendsThrowable(notException), but the catch order here means theExceptionblock at Line 53 won't catch Guzzle exceptions anyway. However, both blocks have identical handling.Consider consolidating into a single catch for
Throwable:Simplified exception handling
- } catch (Exception $e) { - $error = 'Invalid credentials or server error: ' . $e->getMessage(); - } catch (GuzzleException $e) { + } catch (Exception | GuzzleException $e) { $error = 'Invalid credentials or server error: ' . $e->getMessage(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/AuthController.php` around lines 53 - 57, Consolidate the redundant exception handlers in AuthController by replacing the two identical catches with a single catch(Throwable $e) (or a combined union type) so all errors including GuzzleException are handled uniformly; update the catch block that sets $error = 'Invalid credentials or server error: ' . $e->getMessage(); and remove the duplicate catch for GuzzleException to keep the error handling concise and correct.
68-71: Consider handling logout API errors.If
authClient->logout()fails, the user is still logged out locally (session cleared) but the API token might remain valid. Depending on security requirements, you might want to log failures or show a warning.For most use cases, silent failure is acceptable here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/AuthController.php` around lines 68 - 71, In AuthController, wrap the call to $this->authClient->logout() in error handling so failures are not silently ignored: catch exceptions (or check the logout result) from authClient->logout() and log the error via your logger (or add a user-facing flash notice) after the session removals ($request->getSession()->remove('auth_token') / remove('auth_id')). Ensure you reference AuthController's authClient->logout() and getSession() so the handler records the failure (and optionally retains behavior of local logout) without changing session-clearing order.assets/vue/components/subscribers/SubscriberDirectory.vue (3)
230-237: Debounce works but consider cleanup.The debounce pattern is fine, but
searchTimeoutisn't cleared on component unmount. For long-lived SPAs this is usually fine, but you could add anonUnmountedhook to clear pending timeouts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/subscribers/SubscriberDirectory.vue` around lines 230 - 237, The debounce uses a module-level/closure variable searchTimeout in handleSearch but never clears it on component teardown; add an onUnmounted hook that checks and clears searchTimeout (clearTimeout(searchTimeout) and set searchTimeout = null) to prevent pending callbacks calling fetchSubscribers after the component is destroyed; locate handleSearch, searchTimeout and fetchSubscribers in SubscriberDirectory.vue and add the onUnmounted cleanup referencing the same searchTimeout variable.
290-294: HardcodedlistId: 0in import.The import always uses
listId: 0. If subscribers should be imported into a specific list, this should be configurable. If 0 means "no list" by design, consider adding a comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/subscribers/SubscriberDirectory.vue` around lines 290 - 294, The import call in SubscriberDirectory.vue hardcodes listId: 0; change it to use a configurable value (e.g., a reactive selectedListId, a component prop like importListId, or the currently selected list from your store) and pass that variable into subscribersClient.importSubscribers instead of 0 (or, if 0 is intentionally used to mean “no list”, replace the literal with a well-named constant like NO_LIST_ID and add a comment explaining the semantics). Update any UI/state (e.g., a dropdown or prop) to allow selecting the target list and ensure importSubscribers is invoked with that selectedListId (or the named constant) so imports go to the intended list.
306-322: Remove theexportSubscribersfunction or wire it up to an export button.The function is implemented but has zero references in the codebase—it's dead code. Either remove it entirely or add a UI button that calls it. As is, it's just taking up space.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/subscribers/SubscriberDirectory.vue` around lines 306 - 322, The exportSubscribers function is unused dead code; either delete the exportSubscribers function from SubscriberDirectory.vue, or add a clickable UI element that invokes it (e.g., a button in the component template with `@click`="exportSubscribers"). If you choose wiring, ensure the method is exported in the component's methods/setup (exportSubscribers) and the template button is only shown when export is allowed (respecting currentFilter/searchColumn/pagination state) so it triggers the existing params->window.location.href logic correctly.tests/Unit/Controller/AuthControllerTest.php (1)
217-240: Good coverage for the newabout()endpoint.Test properly mocks the Administrator entity and verifies the JsonResponse content. Covers the happy path well.
Consider adding a test for the error path (when
getSessionUser()throws) to verify the 503 response.Would you like me to draft an error-path test case for
testAboutError()?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Controller/AuthControllerTest.php` around lines 217 - 240, Add a new test method (e.g., testAboutError) in AuthControllerTest to cover the error path when authClient->getSessionUser throws: mock $this->authClient->expects($this->once())->method('getSessionUser')->willThrowException(new \Exception('some error')), call $this->controller->about(), and assert the response is a JsonResponse with HTTP 503 status and the expected error payload/message used by the controller; reference the existing testAbout, about(), getSessionUser, $this->authClient and $this->controller to place and structure the new test similarly.src/Controller/SubscribersController.php (3)
95-102: Filter building logic is duplicated.The
SubscribersFilterRequestconstruction is identical to lines 42-49 inindex(). Extracting to a private helper would keep things DRY.♻️ Extract to private method
private function buildFilterFromRequest(Request $request): SubscribersFilterRequest { return new SubscribersFilterRequest( isConfirmed: $request->query->has('confirmed') ? true : ($request->query->has('unconfirmed') ? false : null), isBlacklisted: $request->query->has('blacklisted') ? true : ($request->query->has('non-blacklisted') ? false : null), findColumn: $request->query->get('findColumn'), findValue: $request->query->get('findValue'), ); }Then use
$filter = $this->buildFilterFromRequest($request);in both methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/SubscribersController.php` around lines 95 - 102, Duplicate SubscribersFilterRequest construction exists in index() and another method; extract the logic into a private helper (e.g., private function buildFilterFromRequest(Request $request): SubscribersFilterRequest) that returns the new SubscribersFilterRequest with the same isConfirmed, isBlacklisted, findColumn, and findValue logic, then replace both original constructions with $filter = $this->buildFilterFromRequest($request); to keep SubscribersController DRY.
109-109: No error handling if fopen fails.
fopen()can returnfalseon failure. Whilephp://temprarely fails, defensive coding would check before callingfputcsv.🛡️ Defensive check
$handle = fopen('php://temp', 'r+'); + if ($handle === false) { + return new Response('Failed to create export file.', Response::HTTP_INTERNAL_SERVER_ERROR); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/SubscribersController.php` at line 109, The fopen call for 'php://temp' in SubscribersController (creating $handle via fopen('php://temp', 'r+')) lacks error handling; verify that $handle is not false before using it with fputcsv/rewind/stream_get_contents, and if fopen fails handle the error (e.g., log via the controller logger, throw an exception, or return an error response) and ensure any resources are cleaned up only when $handle is a valid resource.
53-65: Pagination history could grow unbounded.The
subscribers_historysession array keeps growing as users paginate. For long sessions, this could get chunky. Consider capping it or clearing on filter changes.💡 Optional: limit history size
$history = $request->getSession()->get('subscribers_history', []); if (!in_array($afterId, $history, true)) { $history[] = $afterId; + // Keep only last 100 entries to prevent unbounded growth + if (count($history) > 100) { + $history = array_slice($history, -100); + } $request->getSession()->set('subscribers_history', $history); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/SubscribersController.php` around lines 53 - 65, The session-held subscribers_history array ($history) is unbounded; update the code in SubscribersController (where $history, $afterId, $prevId and array_search are used) to cap its size and/or clear it on filter changes: after adding $afterId to $history via getSession()->set('subscribers_history', $history) trim the array to a fixed MAX_HISTORY (e.g. keep only the last N entries using array_slice or repeated array_shift) and store it back, and also detect filter changes (compare a stored filter signature in session or request parameters) and reset/replace 'subscribers_history' when filters change so old pagination entries are cleared.
🤖 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/styles/app.css`:
- Around line 1-2: Remove the obsolete Tailwind v3 directive from your
stylesheet by deleting the line containing "@tailwind utilities" and keep only
the "@import \"tailwindcss\"" directive in the top-level stylesheet (locate the
import block in assets/styles/app.css or the file that contains `@import`
"tailwindcss" and `@tailwind` utilities). Also update your Stylelint config to
avoid false positives: modify the at-rule-no-unknown (or
scss/at-rule-no-unknown) rule to include an ignoreAtRules array with
['theme','source','apply','utility','variant','plugin','config'] or
alternatively install/extend a Tailwind v4-aware config such as
stylelint-config-tailwindcss so Tailwind v4 at-rules are recognized.
In `@assets/vue/api.js`:
- Around line 3-11: When apiBaseUrl is missing (derived from
appElement?.dataset.apiBaseUrl), do not proceed to construct the Client with an
empty string; instead fail fast by throwing an Error or returning early so the
app won't continue in a broken state. Update the module initialization around
apiBaseUrl, apiToken, appElement and the creation of new Client(...) to either
throw a descriptive Error (e.g., including that apiBaseUrl is required) or
prevent creating Client when apiBaseUrl is falsy, ensuring any calling code
cannot proceed with an empty base URL.
In `@assets/vue/components/base/BaseProgressBar.vue`:
- Around line 27-29: wrapperStyle is a plain object created once and won't
update when props.height changes; change wrapperStyle to a reactive computed
that returns { height: props.height } so the style updates when the height prop
changes (update the symbol wrapperStyle in BaseProgressBar.vue to be a computed
using props.height).
In `@assets/vue/components/sidebar/AppSidebar.vue`:
- Around line 46-79: The sidebar's route strings in the sections constant
(sections -> items -> route) don't match the application's registered routes;
update each sections[].items[].route to the exact path or named route used by
the router module (or add the missing route definitions in the router) so clicks
don't fall through to the catch-all; locate the sections array in AppSidebar.vue
and either (A) change every route value (e.g., '/', '/subscribers', '/lists',
'/campaigns', '/templates', '/bounces', '/analytics', '/settings', '/public') to
the canonical paths or named-route identifiers used by the router, or (B) add
corresponding route definitions in the router configuration with those paths so
they resolve correctly; as a temporary safe fallback, set any unresolved items'
route to null or a safe placeholder (so the component can render them disabled)
until the router is updated.
In `@assets/vue/components/subscribers/ImportResult.vue`:
- Line 53: The hover utility uses a non-existent Tailwind color class
`bg-ext-wf3` in the ImportResult.vue button class string, so the hover won't
apply; fix by either adding `ext-wf3` to your Tailwind config colors (e.g., a
darker shade matching `ext-wf1`) or replace `bg-ext-wf3` in ImportResult.vue
with an existing configured color class (for example another `ext-*` shade or a
default Tailwind color) so the hover state resolves correctly; update the
Tailwind config colors block if you add the new token and rebuild CSS so
`bg-ext-wf3` becomes available.
In `@assets/vue/components/subscribers/SubscriberDirectory.vue`:
- Line 37: The hover class on the button in SubscriberDirectory.vue uses an
undefined Tailwind color token `bg-ext-wf3` (same issue as ImportResult.vue);
either replace `bg-ext-wf3` with an existing Tailwind color utility (e.g., a
defined bg-ext-wf2 or a standard color like bg-gray-200) in the class string on
the element using that long class list, or add `ext-wf3` to your Tailwind
config's theme.colors (and rebuild) so `bg-ext-wf3` becomes valid; update any
other occurrences (e.g., ImportResult.vue) to keep styles consistent.
In `@assets/vue/components/subscribers/SubscriberModal.vue`:
- Around line 113-129: The Save button is outside the form so pressing Enter in
inputs (e.g., email) doesn't submit; update the template so the Save button
submits the form by either moving the buttons div into the <form> element that
contains the subscriber fields or keep it outside but add form="subscriber-form"
to the Save button and ensure the form element has id="subscriber-form"; also
confirm the save method (save) remains the submit handler (or change to `@submit`
on the form to call save) and leave the Cancel button (close) behavior
unchanged.
In `@assets/vue/components/subscribers/SubscribersTable.vue`:
- Around line 1-64: This file SubscribersTable.vue is a duplicate of
CampaignsTable.vue (renders campaign columns, uses campaign statuses via the
statusClass function, and has an incorrect empty message) and is unused; remove
the file to avoid confusion, or if you intended it to serve campaigns instead,
move/rename it to the proper location and name (e.g.,
dashboard/CampaignsTable.vue), update the component name/imports where used, and
fix the empty-state text and column headings (or else replace its implementation
with the real SubscriberTable.vue structure which expects ID, Email, Status,
Lists, Created) so that props (rows) and statusClass remain consistent with the
intended table.
In `@assets/vue/components/subscribers/SubscriberTable.vue`:
- Around line 82-84: The mobile status text in the template span using the
expression subscriber.confirmed ? 'active' : 'unconfirmed' is inconsistent with
the desktop wording ("Confirmed" / "Unconfirmed"); change that ternary to output
the same capitalized strings (e.g., 'Confirmed' : 'Unconfirmed') so the mobile
view matches the desktop status wording and casing in SubscriberTable.vue.
In `@assets/vue/layouts/AdminLayout.vue`:
- Around line 35-38: Replace the non-semantic <div> used as the dropdown trigger
with a <button type="button"> while keeping the same class list and
`@click`="toggleDropdown"; add ARIA attributes aria-haspopup="true" and bind
aria-expanded to the dropdown state (e.g., :aria-expanded="isOpen" or the
component's dropdown boolean) so screen readers see the state, and ensure the
existing toggleDropdown method (toggleDropdown) remains the click handler;
remove any JS-only keyboard handling since a native button supports Enter/Space
and preserve visual focus styles for accessibility.
In `@composer.json`:
- Around line 19-22: The composer.json currently lists a VCS repository entry
for "https://github.com/tatevikgr/rss-bundle.git" but the package
"tatevikgr/rss-bundle" is not present in composer.lock; either remove that
repository entry from composer.json if the bundle is no longer needed, or add
the package to the project's dependencies by running composer require
tatevikgr/rss-bundle (or adding it to "require" in composer.json) and then run
composer update to regenerate composer.lock; locate the repository entry in
composer.json and the dependency name "tatevikgr/rss-bundle" when applying the
change.
In `@config/services.yml`:
- Line 4: The default API_BASE_URL currently falls back to an insecure value;
update the env(DEFAULT/API_BASE_URL) fallback so it does not use plain
HTTP—either change env(API_BASE_URL) to an HTTPS default (e.g., 'https://...')
or remove the insecure default entirely and require callers to supply a value;
ensure the env key env(API_BASE_URL) is updated and documented so no insecure
HTTP fallback remains.
In `@postcss.config.js`:
- Around line 1-6: PostCSS config includes autoprefixer but Tailwind v4's
`@tailwindcss/postcss` handles vendor prefixing; remove the
require('autoprefixer') entry from the plugins array in postcss.config.js so
plugins only contains require("@tailwindcss/postcss"), ensuring no other code
paths reference autoprefixer and update any comments accordingly.
In `@src/Controller/SubscribersController.php`:
- Line 136: The current mapping in SubscribersController (the 'lists' =>
implode('|', array_map(...)) expression) assumes every element of
$data->subscribedLists has a 'name' key and will raise undefined index warnings;
update the transformation to first filter out entries missing 'name' (or map
using a null-coalescing fallback) and only pass valid names to implode — locate
the array_map closure around $data->subscribedLists and replace it with a safe
pipeline that either filters with isset($list['name']) or uses $list['name'] ??
'' and then removes empty strings before calling implode so no undefined index
warnings occur.
- Line 31: The Accept header check in SubscribersController (the if that uses
$request->headers->get('Accept') !== 'application/json') is too strict and will
miss valid JSON requests; update the condition to detect JSON more flexibly (for
example use $request->getAcceptableContentTypes() and check for
'application/json' in that array or use strpos on the Accept header) so the
branch that currently tests isXmlHttpRequest() && Accept covers cases like
"application/json, text/html"; modify the if in the controller method
accordingly (keep referencing $request, isXmlHttpRequest(), and the Accept
header check).
- Line 104: The call in SubscribersController using
$request->query->getInt('limit') passes 0 when the parameter is absent which can
cause getSubscribers to return no results; update the SubscribersController
invocation to supply a sensible default (or validate/require the param) before
calling $this->subscribersClient->getSubscribers — e.g. read the limit with
getInt('limit', <defaultLimit>) or compute a default (like 100 or a configured
value) and pass that variable into getSubscribers, and if you choose to require
the param, return a 4xx error when missing instead of using 0.
- Line 74: The DateTimeImmutable construction for $subscriber->createdAt in
SubscribersController (and the similar usage in export) can throw on
null/malformed input; update the code around the 'createdAt' formatting to
defensively handle invalid values by checking if $subscriber->createdAt is
truthy and a valid date string (or wrap the new DateTimeImmutable(...) call in a
try-catch), and on failure return a safe fallback (e.g., null or an empty
string) instead of letting an exception bubble; apply the same defensive
check/try-catch to the export method's createdAt formatting to ensure both code
paths never throw on bad input.
In `@src/EventSubscriber/UnauthorizedSubscriber.php`:
- Around line 36-59: The flash is being added after calling
$session->invalidate() so it may be lost; change the logic in
UnauthorizedSubscriber (around where $request->hasSession(),
$session->invalidate(), $request->isXmlHttpRequest(), and
session->getFlashBag()->add(...) are used) to add the flash message to the
session (via $session->getFlashBag()->add('error', ...)) before calling
$session->invalidate(), and ensure the AJAX branch (where
$event->setResponse(new JsonResponse(...))) still returns immediately without
trying to add a flash; reorder the operations so non-AJAX requests receive the
flash prior to invalidation.
---
Duplicate comments:
In `@templates/auth/login.html.twig`:
- Line 38: The "Forgot password?" anchor uses a placeholder href="#" and must be
wired to a real route or removed; update the anchor in
templates/auth/login.html.twig so its href calls the correct password-reset
route (e.g., use Twig's path('password_reset') or the actual route name used by
your app) or remove the <a> entirely if password reset isn't implemented yet,
and ensure the class/text remain unchanged.
- Around line 29-52: The login form in login.html.twig is missing a CSRF token
input; add the framework's CSRF hidden field inside the <form> (e.g., insert the
Twig CSRF token helper or a hidden input named like csrf_token) and ensure the
server-side login handler (the route/controller that processes the form
submission) validates this token before authenticating the user; reference the
form element in the template and the login handler method to implement token
generation, insertion, and validation.
In `@templates/base.html.twig`:
- Around line 12-14: The template currently double-loads the app bundle: keep
the Twig block that calls encore_entry_script_tags('app') inside the javascripts
block and remove the duplicate manual <script> tags that statically include
runtime.js and app.js at the end of the body; locate the static tags near the
bottom of templates/base.html.twig and delete them so the Vue app is only
initialized via encore_entry_script_tags('app').
---
Nitpick comments:
In `@apache/web-frontend.conf`:
- Around line 21-25: The SetHandler in the FilesMatch block currently hardcodes
the PHP-FPM socket path "proxy:unix:/run/php/php8.1-fpm.sock|fcgi://localhost",
which will break if PHP is upgraded; update the FilesMatch/SetHandler usage to
reference a configurable Define or environment-provided variable (e.g.,
PHP_FPM_SOCK) instead of the literal path, or document the required PHP version
and instruct maintainers to create a stable symlink (/run/php/php-fpm.sock) that
points to the active version; adjust the SetHandler reference in the FilesMatch
block accordingly so the socket path is not tied to "php8.1" directly.
- Around line 1-2: This VirtualHost is configured for HTTP only (VirtualHost
*:80 with ServerName frontend.phplist.local); if this is only for local dev, add
a clear comment stating that and restrict access (e.g., bind to localhost or
local-only network), otherwise create a matching HTTPS VirtualHost (VirtualHost
*:443) with TLS settings (certificate, SSLEngine on) or ensure a production
reverse proxy handles TLS so production is not served over plain HTTP.
In `@assets/vue/components/dashboard/CampaignsTable.vue`:
- Around line 47-53: The code assigns defineProps(...) to a never-used variable
"props"; remove the unused variable by replacing "const props = defineProps({
... })" with a direct call "defineProps({ ... })" so props are declared for the
component without creating an unused identifier, and verify that statusClass
continues to accept a status parameter (no change needed) rather than trying to
read props.rows.
In `@assets/vue/components/dashboard/RecentCampaignsCard.vue`:
- Around line 20-51: Replace the baked-in mock campaign data in the
RecentCampaignsCard.vue props so the component no longer defaults to stale
samples: change the defineProps({ rows: { ... default: (...) }}) to default to
an empty array (or remove the default) and ensure the component
(RecentCampaignsCard) handles an empty rows array via its empty state; then wire
DashboardView to fetch real campaign data from the backend and pass it into the
rows prop (or open an issue to track adding a fetch in DashboardView if you
prefer to defer implementation).
In `@assets/vue/components/sidebar/SidebarLogo.vue`:
- Around line 4-9: The img in SidebarLogo.vue has conflicting Tailwind height
classes on the same element (class="... h-full h-auto ..."); decide the intended
behavior and remove the other class: if you want the logo to fill its container
keep h-full and remove h-auto, or if you want natural aspect ratio keep h-auto
and remove h-full; update the class attribute on the <img> in the SidebarLogo
component accordingly.
In `@assets/vue/components/sidebar/SidebarNavSection.vue`:
- Around line 6-8: The v-for in SidebarNavSection.vue uses a fragile key
(:key="item.label"); change the key to a stable unique identifier such as
:key="item.route" or an explicit :key="item.id" (ensure each item in items has
that property) so the list rendering for items remains stable and avoids
patching glitches; update any data construction that builds items to include a
unique id if necessary.
In `@assets/vue/components/subscribers/ImportResult.vue`:
- Around line 71-79: The importResult prop currently declares required: true
while also providing a default value, which is redundant; decide whether this
component should accept a missing prop or enforce it—either remove the default
function from the importResult prop to keep required: true, or remove required:
true and keep the default object ({ imported: 0, skipped: 0, errors: []}) so the
component can operate without the parent passing importResult; update the
importResult prop declaration accordingly in ImportResult.vue.
In `@assets/vue/components/subscribers/SubscriberDirectory.vue`:
- Around line 230-237: The debounce uses a module-level/closure variable
searchTimeout in handleSearch but never clears it on component teardown; add an
onUnmounted hook that checks and clears searchTimeout
(clearTimeout(searchTimeout) and set searchTimeout = null) to prevent pending
callbacks calling fetchSubscribers after the component is destroyed; locate
handleSearch, searchTimeout and fetchSubscribers in SubscriberDirectory.vue and
add the onUnmounted cleanup referencing the same searchTimeout variable.
- Around line 290-294: The import call in SubscriberDirectory.vue hardcodes
listId: 0; change it to use a configurable value (e.g., a reactive
selectedListId, a component prop like importListId, or the currently selected
list from your store) and pass that variable into
subscribersClient.importSubscribers instead of 0 (or, if 0 is intentionally used
to mean “no list”, replace the literal with a well-named constant like
NO_LIST_ID and add a comment explaining the semantics). Update any UI/state
(e.g., a dropdown or prop) to allow selecting the target list and ensure
importSubscribers is invoked with that selectedListId (or the named constant) so
imports go to the intended list.
- Around line 306-322: The exportSubscribers function is unused dead code;
either delete the exportSubscribers function from SubscriberDirectory.vue, or
add a clickable UI element that invokes it (e.g., a button in the component
template with `@click`="exportSubscribers"). If you choose wiring, ensure the
method is exported in the component's methods/setup (exportSubscribers) and the
template button is only shown when export is allowed (respecting
currentFilter/searchColumn/pagination state) so it triggers the existing
params->window.location.href logic correctly.
In `@assets/vue/components/subscribers/SubscriberFilters.vue`:
- Around line 27-35: The current toggleFilter function sets activeFilter.value
to null when the user clicks the already-active filter, creating a "no
selection" state; change toggleFilter so that when activeFilter.value ===
filterId it falls back to the default 'all' (i.e., activeFilter.value = 'all')
instead of null, and continue to emit('filter-change', activeFilter.value) so
the parent always receives a valid filter value; update the logic in the
toggleFilter function and any usages relying on null to expect 'all' instead.
In `@assets/vue/components/subscribers/SubscriberModal.vue`:
- Around line 202-204: The close() handler currently only emits 'close' but
leaves local state (subscriber, formData, error) intact, causing stale values to
flash if the modal is reopened; update the close function to reset subscriber to
its initial empty value, reset formData to the initial form shape, and clear
error before emitting 'close' (or emit after resetting) so the modal always
starts with a clean state; reference the close() function and the subscriber,
formData, and error reactive/state variables when making this change.
In `@assets/vue/components/subscribers/SubscriberTable.vue`:
- Around line 130-139: The helper function formatDate is dead code because the
template renders raw dates with {{ subscriber.createdAt }}; either remove
formatDate or wire it into the template so dates are formatted. To fix, either
delete the unused formatDate function, or update the template where
subscriber.createdAt is displayed to call the formatter (e.g., use the
formatDate helper or map it via a computed/ method) so createdAt is passed to
formatDate (and optionally set isIso) before rendering; ensure any
import/registration needed for formatDate in the component options is added if
you choose to keep it.
- Line 112: The file imports Vue's inject but doesn't use it; remove the unused
import to clean up SubscriberTable.vue by deleting "inject" from the import
statement (or remove the entire import line if nothing else is imported),
ensuring no references to inject remain in the component (check for symbols like
inject or provide/inject usage) before committing.
In `@composer.json`:
- Around line 50-54: The composer.json currently depends on moving branches
"phplist/core": "dev-dev" and "tatevikgr/rest-api-client": "dev-main"; replace
those unstable refs with either a specific tagged version (e.g., "1.2.3") or a
commit-pinned ref (e.g., "dev-main#<commit-hash>") for production releases,
update the dependency entries for phplist/core and tatevikgr/rest-api-client
accordingly, regenerate and commit composer.lock, and document in the repo
(README or RELEASE notes) the chosen commit hashes or tags that are known to
work.
In `@package.json`:
- Line 24: Update the package.json dependency entry for "vue-router" to pin a
more specific version to avoid pulling unintended minor releases; locate the
"vue-router" entry in package.json and change its version string from "4" to a
fixed or more conservative range such as "~4.6.4" (or "^4.6.4" if you prefer
less strictness) so only safe patch updates are allowed.
In `@src/Controller/AuthController.php`:
- Around line 53-57: Consolidate the redundant exception handlers in
AuthController by replacing the two identical catches with a single
catch(Throwable $e) (or a combined union type) so all errors including
GuzzleException are handled uniformly; update the catch block that sets $error =
'Invalid credentials or server error: ' . $e->getMessage(); and remove the
duplicate catch for GuzzleException to keep the error handling concise and
correct.
- Around line 68-71: In AuthController, wrap the call to
$this->authClient->logout() in error handling so failures are not silently
ignored: catch exceptions (or check the logout result) from authClient->logout()
and log the error via your logger (or add a user-facing flash notice) after the
session removals ($request->getSession()->remove('auth_token') /
remove('auth_id')). Ensure you reference AuthController's authClient->logout()
and getSession() so the handler records the failure (and optionally retains
behavior of local logout) without changing session-clearing order.
In `@src/Controller/SubscribersController.php`:
- Around line 95-102: Duplicate SubscribersFilterRequest construction exists in
index() and another method; extract the logic into a private helper (e.g.,
private function buildFilterFromRequest(Request $request):
SubscribersFilterRequest) that returns the new SubscribersFilterRequest with the
same isConfirmed, isBlacklisted, findColumn, and findValue logic, then replace
both original constructions with $filter =
$this->buildFilterFromRequest($request); to keep SubscribersController DRY.
- Line 109: The fopen call for 'php://temp' in SubscribersController (creating
$handle via fopen('php://temp', 'r+')) lacks error handling; verify that $handle
is not false before using it with fputcsv/rewind/stream_get_contents, and if
fopen fails handle the error (e.g., log via the controller logger, throw an
exception, or return an error response) and ensure any resources are cleaned up
only when $handle is a valid resource.
- Around line 53-65: The session-held subscribers_history array ($history) is
unbounded; update the code in SubscribersController (where $history, $afterId,
$prevId and array_search are used) to cap its size and/or clear it on filter
changes: after adding $afterId to $history via
getSession()->set('subscribers_history', $history) trim the array to a fixed
MAX_HISTORY (e.g. keep only the last N entries using array_slice or repeated
array_shift) and store it back, and also detect filter changes (compare a stored
filter signature in session or request parameters) and reset/replace
'subscribers_history' when filters change so old pagination entries are cleared.
In `@tailwind.config.js`:
- Around line 6-8: The content globs in tailwind.config.js are redundant: remove
the single-star ("./assets/vue/*.vue") and the triple-star
("./assets/vue/**/**/*.vue") entries and keep the recursive double-star pattern
("./assets/vue/**/*.vue") only; update the exported content array so it contains
the simplified pattern to avoid duplicate scanning and potential maintenance
confusion.
- Around line 10-17: You replaced Tailwind's default breakpoints by declaring
theme.screens directly; if you want to keep the defaults (including 2xl at
1536px) move your custom screens into theme.extend.screens instead of
theme.screens so Tailwind merges them rather than overwriting—update the screens
definition used in the tailwind.config.js (the theme and screens keys) to
theme.extend.screens to preserve built-in breakpoints.
In `@templates/base.html.twig`:
- Line 8: The template is potentially loading the same stylesheet twice: remove
the direct <link rel="stylesheet" href="{{ asset('build/app.css') }}"> if your
Webpack Encore entry uses encore_entry_link_tags('styles'); inspect the Encore
entry named 'styles' (encore_entry_link_tags('styles')) and, if it outputs
build/app.css, delete the asset() link to avoid duplicate CSS; if the Encore
entry does not include that file, keep the asset() link or adjust the Encore
configuration so the stylesheet is emitted via the 'styles' entry.
In `@tests/Integration/Auth/LoginTest.php`:
- Around line 23-26: Both tests use inconsistent Panther client connection
timeouts; update the createPantherClient calls in LoginTest (used in
testLoginPageFormFieldsAreVisible and testLoginFormSubmission) so they use the
same timeout value (choose the longer 20000ms for reliability) or define a
shared constant (e.g., DEFAULT_CONNECTION_TIMEOUT_MS) and use it in both places
to avoid flaky runs.
- Line 55: The test uses a brittle Tailwind selector in LoginTest.php (the
assertSelectorExists call with '.bg-red-50.border-red-200.text-red-600'); change
the app template to add a stable test hook (e.g., data-testid="login-error" on
the error alert div) and update the test to
assertSelectorExists('[data-testid="login-error"]') instead of the utility-class
selector so styling changes won't break the test.
In `@tests/Unit/Controller/AuthControllerTest.php`:
- Around line 217-240: Add a new test method (e.g., testAboutError) in
AuthControllerTest to cover the error path when authClient->getSessionUser
throws: mock
$this->authClient->expects($this->once())->method('getSessionUser')->willThrowException(new
\Exception('some error')), call $this->controller->about(), and assert the
response is a JsonResponse with HTTP 503 status and the expected error
payload/message used by the controller; reference the existing testAbout,
about(), getSessionUser, $this->authClient and $this->controller to place and
structure the new test similarly.
Comment on lines
+1
to
+2
| @import "tailwindcss"; | ||
| @tailwind utilities; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How to configure Stylelint to ignore Tailwind CSS v4 at-rules like @themeand@source?
💡 Result:
To configure Stylelint to ignore Tailwind CSS v4 at-rules like @theme and @source, update your Stylelint configuration file (e.g., stylelint.config.js) to override the "at-rule-no-unknown" rule and list the Tailwind v4 at-rules in the ignoreAtRules array. Example configuration: js module.exports = { extends: [ 'stylelint-config-standard', // or your preferred base config ], rules: { 'at-rule-no-unknown': [ true, { ignoreAtRules: [ 'theme', 'source', 'apply', 'utility', 'variant', 'plugin', 'config', 'reference', 'custom-variant' ] } ] } }; If using SCSS, also disable the base rule and configure the SCSS-specific one: js rules: { 'at-rule-no-unknown': null, 'scss/at-rule-no-unknown': [ true, { ignoreAtRules: [ 'theme', 'source', // ... other rules ] } ] } Optionally, install and extend a Tailwind-specific config like stylelint-config-tailwindcss (npm install --save-dev stylelint-config-tailwindcss), though it may need updates for full v4 support: js { extends: [ 'stylelint-config-recommended', 'stylelint-config-tailwindcss' ] } This prevents Stylelint from flagging Tailwind v4's custom at-rules as unknown. Run stylelint on your CSS files to verify.
Citations:
- 1: https://stackoverflow.com/questions/72161637/unexpected-unknown-at-rule-tailwind-scss-at-rule-no-unknown
- 2: https://coldfusion-example.blogspot.com/2025/12/tailwind-css-v4-upgrade-fixing-unknown.html
- 3: https://tailkits.com/blog/how-to-fix-unknown-at-rule-error-in-tailwind-css/
- 4: https://gist.github.com/stctheproducer/332e1171a1f4eb6647933bc8d3f8aaf6
- 5: https://tailwindcss.com/docs/functions-and-directives
- 6: https://www.w3tutorials.net/blog/unexpected-unknown-at-rule-tailwind-scss-at-rule-no-unknown/
- 7: https://stylelint.io/user-guide/rules/at-rule-no-unknown
🌐 Web query:
Tailwind CSS v4 @import "tailwindcss" does it include utilities automatically
💡 Result:
Yes, using @import "tailwindcss"; in Tailwind CSS v4 automatically includes all Tailwind utilities (as well as base styles and theme), replacing the old @tailwind base; @tailwind components; @tailwind utilities; directives from v3. You can then use utility classes like text-xl, bg-blue-500, etc. directly in your HTML.
Citations:
- 1: https://tailwindcss.com/blog/tailwindcss-v4
- 2: https://tailwindcss.com/docs/functions-and-directives/
- 3: https://tailwindcss.com/docs/styling-with-utility-classes
- 4: Tailwind v4 has no `important` option support tailwindlabs/tailwindcss#15866
- 5: https://v3.tailwindcss.com/docs/v4-beta
- 6: https://tailwindcss.com/docs/v4-beta
- 7: https://tailwindcss.com/docs/upgrade-guide
🌐 Web query:
Tailwind CSS v4 @tailwindutilities duplicate if used with@import tailwindcss
💡 Result:
No, using @tailwind utilities alongside @import "tailwindcss" does not cause duplicates in Tailwind CSS v4. The @tailwind directives (base, components, utilities) from v3 have been completely removed in v4. The correct and only way to include Tailwind CSS v4 is with a single @import "tailwindcss"; line at the top of your CSS file. Using any @tailwind directive will result in errors or no styles being generated, as confirmed by official docs and upgrade guide. The perceived "duplicate" issue likely stems from misconfigurations during migration or runtime setups where Tailwind is imported/processed multiple times (e.g., build-time CSS + runtime @import "tailwindcss" source(none); without deduplication), leading to regenerated utilities. This is a user setup problem, not inherent to @tailwind utilities with @import. To fix: - Replace all @tailwind base; @tailwind components; @tailwind utilities; with @import "tailwindcss"; - Use npx @tailwindcss/upgrade for automated migration. - For runtime generation without duplicates, avoid re-importing full Tailwind; use targeted @source or @import "tailwindcss/utilities.css" layer(utilities); with source(none) carefully, as shown in GitHub discussions.
Citations:
- 1: https://tailwindcss.com/blog/tailwindcss-v4
- 2: https://tailwindcss.com/docs/functions-and-directives
- 3: https://tailwindcss.com/docs/upgrade-guide
- 4: How to Prevent Duplicate CSS When Merging Tailwind 4 Output at Build and Runtime? tailwindlabs/tailwindcss#18526
- 5: @tailwind base and @tailwind components no longer work in Tailwind v4 tailwindlabs/tailwindcss.com#2076
Remove @tailwind utilities — it's not valid in Tailwind CSS v4.
In Tailwind CSS v4, @import "tailwindcss" is the only directive you need. The @tailwind directives from v3 (base, components, utilities) have been completely removed. Having @tailwind utilities on line 2 will cause errors or result in no styles being applied.
🧹 Proposed fix
`@import` "tailwindcss";
-@tailwind utilities;For the Stylelint false positives, the web search confirms you can suppress them by updating your Stylelint config to recognize Tailwind v4's at-rules. Add an ignoreAtRules array to the at-rule-no-unknown rule (or scss/at-rule-no-unknown for SCSS) with ['theme', 'source', 'apply', 'utility', 'variant', 'plugin', 'config'], or install and extend stylelint-config-tailwindcss if available for your v4 setup.
📝 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
| @import "tailwindcss"; | |
| @tailwind utilities; | |
| `@import` "tailwindcss"; |
🧰 Tools
🪛 Stylelint (17.4.0)
[error] 2-2: Unexpected unknown at-rule "@tailwind" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/styles/app.css` around lines 1 - 2, Remove the obsolete Tailwind v3
directive from your stylesheet by deleting the line containing "@tailwind
utilities" and keep only the "@import \"tailwindcss\"" directive in the
top-level stylesheet (locate the import block in assets/styles/app.css or the
file that contains `@import` "tailwindcss" and `@tailwind` utilities). Also update
your Stylelint config to avoid false positives: modify the at-rule-no-unknown
(or scss/at-rule-no-unknown) rule to include an ignoreAtRules array with
['theme','source','apply','utility','variant','plugin','config'] or
alternatively install/extend a Tailwind v4-aware config such as
stylelint-config-tailwindcss so Tailwind v4 at-rules are recognized.
Comment on lines
+3
to
+11
| const appElement = document.getElementById('vue-app'); | ||
| const apiToken = appElement?.dataset.apiToken; | ||
| const apiBaseUrl = appElement?.dataset.apiBaseUrl; | ||
|
|
||
| if (!apiBaseUrl) { | ||
| console.error('API Base URL is not configured.'); | ||
| } | ||
|
|
||
| const client = new Client(apiBaseUrl || ''); |
There was a problem hiding this comment.
Proceeding with empty URL after error log can cause confusing failures.
When apiBaseUrl is missing, the code logs an error but then continues with new Client(''). This means API calls will silently fail or hit unexpected endpoints rather than failing fast with a clear message.
Consider throwing or returning early so the app doesn't limp along in a broken state:
🛠️ Suggested fix
if (!apiBaseUrl) {
console.error('API Base URL is not configured.');
+ throw new Error('API Base URL is required for the application to function.');
}
-const client = new Client(apiBaseUrl || '');
+const client = new Client(apiBaseUrl);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const appElement = document.getElementById('vue-app'); | |
| const apiToken = appElement?.dataset.apiToken; | |
| const apiBaseUrl = appElement?.dataset.apiBaseUrl; | |
| if (!apiBaseUrl) { | |
| console.error('API Base URL is not configured.'); | |
| } | |
| const client = new Client(apiBaseUrl || ''); | |
| const appElement = document.getElementById('vue-app'); | |
| const apiToken = appElement?.dataset.apiToken; | |
| const apiBaseUrl = appElement?.dataset.apiBaseUrl; | |
| if (!apiBaseUrl) { | |
| console.error('API Base URL is not configured.'); | |
| throw new Error('API Base URL is required for the application to function.'); | |
| } | |
| const client = new Client(apiBaseUrl); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/api.js` around lines 3 - 11, When apiBaseUrl is missing (derived
from appElement?.dataset.apiBaseUrl), do not proceed to construct the Client
with an empty string; instead fail fast by throwing an Error or returning early
so the app won't continue in a broken state. Update the module initialization
around apiBaseUrl, apiToken, appElement and the creation of new Client(...) to
either throw a descriptive Error (e.g., including that apiBaseUrl is required)
or prevent creating Client when apiBaseUrl is falsy, ensuring any calling code
cannot proceed with an empty base URL.
Comment on lines
+27
to
+29
| const wrapperStyle = { | ||
| height: props.height, | ||
| } |
There was a problem hiding this comment.
wrapperStyle won't react to prop changes.
This is a plain object computed once at setup time. If the parent ever updates the height prop dynamically, the style won't reflect the change. Use a computed to keep it reactive.
🔧 Proposed fix
<script setup>
+import { computed } from 'vue'
+
const props = defineProps({
value: {
type: Number,
default: 0,
},
height: {
type: String,
default: "6px", // allows easy overrides: "4px", "10px", etc.
},
})
-const wrapperStyle = {
- height: props.height,
-}
+const wrapperStyle = computed(() => ({
+ height: props.height,
+}))
</script>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const wrapperStyle = { | |
| height: props.height, | |
| } | |
| <script setup> | |
| import { computed } from 'vue' | |
| const props = defineProps({ | |
| value: { | |
| type: Number, | |
| default: 0, | |
| }, | |
| height: { | |
| type: String, | |
| default: "6px", // allows easy overrides: "4px", "10px", etc. | |
| }, | |
| }) | |
| const wrapperStyle = computed(() => ({ | |
| height: props.height, | |
| })) | |
| </script> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/base/BaseProgressBar.vue` around lines 27 - 29,
wrapperStyle is a plain object created once and won't update when props.height
changes; change wrapperStyle to a reactive computed that returns { height:
props.height } so the style updates when the height prop changes (update the
symbol wrapperStyle in BaseProgressBar.vue to be a computed using props.height).
|
|
||
| <div class="px-4 py-3 border-t border-slate-200 flex justify-end"> | ||
| <button | ||
| class="px-4 py-2 bg-ext-wf1 hover:bg-ext-wf3 text-white text-sm font-medium rounded-lg transition-colors" |
There was a problem hiding this comment.
Undefined Tailwind color class bg-ext-wf3.
The hover state uses bg-ext-wf3, but only ext-wf1 is defined in tailwind.config.js. This hover effect won't work.
Either add ext-wf3 to your Tailwind config or use a different existing color.
Option 1: Add color to tailwind.config.js
colors: {
'ext-wf1': '#dc2626',
'ext-wf3': '#b91c1c', // darker shade for hover
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/subscribers/ImportResult.vue` at line 53, The hover
utility uses a non-existent Tailwind color class `bg-ext-wf3` in the
ImportResult.vue button class string, so the hover won't apply; fix by either
adding `ext-wf3` to your Tailwind config colors (e.g., a darker shade matching
`ext-wf1`) or replace `bg-ext-wf3` in ImportResult.vue with an existing
configured color class (for example another `ext-*` shade or a default Tailwind
color) so the hover state resolves correctly; update the Tailwind config colors
block if you add the new token and rebuild CSS so `bg-ext-wf3` becomes
available.
Comment on lines
+36
to
+59
| if ($request->hasSession()) { | ||
| $session = $request->getSession(); | ||
| $session->invalidate(); | ||
| } | ||
|
|
||
| $session->set('login_error', 'Your session has expired. Please log in again.'); | ||
|
|
||
| $loginUrl = $this->urlGenerator->generate('login'); | ||
| $response = new RedirectResponse($loginUrl); | ||
|
|
||
| $event->setResponse($response); | ||
| if ($request->isXmlHttpRequest()) { | ||
| $event->setResponse(new JsonResponse([ | ||
| 'error' => 'session_expired', | ||
| 'message' => 'Your session has expired. Please log in again.', | ||
| 'redirect' => $loginUrl, | ||
| ], 401)); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if ($request->hasSession()) { | ||
| $session = $request->getSession(); | ||
|
|
||
| if (method_exists($session, 'getFlashBag')) { | ||
| $session->getFlashBag()->add('error', 'Your session has expired. Please log in again.'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Flash message added after session invalidation — message may be lost.
The session is invalidated on line 38, which clears session data. Then on lines 53-58, the code tries to add a flash message to the same session. Depending on how Symfony handles this, the flash message could be lost because invalidate() regenerates/clears the session.
Flip the order so the flash message is added before invalidation:
🐛 Proposed fix
if ($exception instanceof AuthenticationException) {
$request = $event->getRequest();
+ $loginUrl = $this->urlGenerator->generate('login');
+ // Add flash message before invalidating session
+ if ($request->hasSession()) {
+ $session = $request->getSession();
+
+ if (method_exists($session, 'getFlashBag')) {
+ $session->getFlashBag()->add('error', 'Your session has expired. Please log in again.');
+ }
+
+ $session->invalidate();
+ }
- if ($request->hasSession()) {
- $session = $request->getSession();
- $session->invalidate();
- }
-
- $loginUrl = $this->urlGenerator->generate('login');
-
if ($request->isXmlHttpRequest()) {
$event->setResponse(new JsonResponse([
'error' => 'session_expired',
'message' => 'Your session has expired. Please log in again.',
'redirect' => $loginUrl,
], 401));
return;
}
- if ($request->hasSession()) {
- $session = $request->getSession();
-
- if (method_exists($session, 'getFlashBag')) {
- $session->getFlashBag()->add('error', 'Your session has expired. Please log in again.');
- }
- }
-
$event->setResponse(new RedirectResponse($loginUrl));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EventSubscriber/UnauthorizedSubscriber.php` around lines 36 - 59, The
flash is being added after calling $session->invalidate() so it may be lost;
change the logic in UnauthorizedSubscriber (around where $request->hasSession(),
$session->invalidate(), $request->isXmlHttpRequest(), and
session->getFlashBag()->add(...) are used) to add the flash message to the
session (via $session->getFlashBag()->add('error', ...)) before calling
$session->invalidate(), and ensure the AJAX branch (where
$event->setResponse(new JsonResponse(...))) still returns immediately without
trying to add a flash; reorder the operations so non-AJAX requests receive the
flash prior to invalidation.
* Dashboard stats from api * VueApexCharts * Quick Actions Card * After review 0 --------- Co-authored-by: Tatevik <tatevikg1@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
assets/vue/views/DashboardView.vue (1)
54-63: Harden payload shape checks before passing to child cards.Right now, any truthy
dashboardStats.chartbypasses defaults (Line 54), even iflabels/seriesare invalid. Add light shape guards to avoid passing malformed data into chart/table components.🛡️ Proposed hardening
-const chart = dashboardStats.chart || { +const defaultChart = { labels: [], series: [ { name: 'Opens', data: [] }, { name: 'Clicks', data: [] }, ], } -const recentCampaigns = dashboardStats.recent_campaigns || [] +const chart = + dashboardStats?.chart && + Array.isArray(dashboardStats.chart.labels) && + Array.isArray(dashboardStats.chart.series) + ? dashboardStats.chart + : defaultChart + +const recentCampaigns = Array.isArray(dashboardStats?.recent_campaigns) + ? dashboardStats.recent_campaigns + : []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/views/DashboardView.vue` around lines 54 - 63, dashboardStats.chart currently accepts any truthy value and passes it to child components; add light runtime shape guards in the component before using it: validate that dashboardStats.chart is an object, dashboardStats.chart.labels is an Array, and dashboardStats.chart.series is an Array of objects where each series has a string name and an Array data; if any check fails, fall back to the existing default chart object (labels: [], series: [{name:'Opens', data:[]}, {name:'Clicks', data:[]}]) and similarly ensure dashboardStats.recent_campaigns is an Array before assigning to recentCampaigns (otherwise use []); update the code paths referencing dashboardStats.chart and recentCampaigns to use these validated variables (e.g., validate around symbols dashboardStats.chart, labels, series, recentCampaigns).assets/vue/components/dashboard/QuickActionsCard.vue (1)
10-15: Switch toRouterLinkfor internal navigation.Line 13 uses
:hreffor internal routes, which triggers full page reloads instead of SPA navigation. Since Vue Router is configured andRouterLinkis already used in other components (likeSidebarNavItem), updateQuickActionsCardto match the pattern for consistency.♻️ Proposed refactor
- <a + <RouterLink v-for="action in quickActions" :key="action.id" - :href="action.href" + :to="action.to" class="rounded-xl border border-gray-200 bg-gray-100/70 px-4 py-4 text-center transition-colors hover:bg-gray-100" > @@ - </a> + </RouterLink> @@ - { id: 'campaign', label: 'New Campaign', href: '/campaigns' }, - { id: 'subscribers', label: 'Add Subscribers', href: '/subscribers' }, - { id: 'import', label: 'Import List', href: '/subscribers' }, - { id: 'templates', label: 'Manage Templates', href: '/templates' }, + { id: 'campaign', label: 'New Campaign', to: '/campaigns' }, + { id: 'subscribers', label: 'Add Subscribers', to: '/subscribers' }, + { id: 'import', label: 'Import List', to: '/subscribers' }, + { id: 'templates', label: 'Manage Templates', to: '/templates' },Also applies to: 83-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/dashboard/QuickActionsCard.vue` around lines 10 - 15, Change the anchor elements generated in QuickActionsCard (v-for over quickActions, key=action.id) to use RouterLink for internal routes: replace <a :href="action.href"> with <RouterLink :to="action.href"> while preserving the class, key, and contents; for external URLs keep a normal <a> (detect by e.g. href starting with 'http' or a boolean flag on action). Ensure RouterLink is imported/registered like other components (consistent with SidebarNavItem) and apply the same change for the second occurrence that iterates actions (lines referenced as 83-88).
🤖 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/dashboard/QuickActionsCard.vue`:
- Around line 85-86: QuickActionsCard.vue defines two actions with ids
'subscribers' and 'import' that both point to '/subscribers', causing the
duplicate destination; either add a dedicated import route (e.g., create a route
with path '/subscribers/import' and a component like ImportSubscribersView or
route name 'SubscribersImport' in your router routes array) and change the
'import' action's href to '/subscribers/import' (or its route name), or remove
the 'import' action from the QuickActionsCard.vue actions array to avoid the
duplicate; update the 'import' item (id: 'import') and corresponding router
entry (routes array / route name) accordingly.
---
Nitpick comments:
In `@assets/vue/components/dashboard/QuickActionsCard.vue`:
- Around line 10-15: Change the anchor elements generated in QuickActionsCard
(v-for over quickActions, key=action.id) to use RouterLink for internal routes:
replace <a :href="action.href"> with <RouterLink :to="action.href"> while
preserving the class, key, and contents; for external URLs keep a normal <a>
(detect by e.g. href starting with 'http' or a boolean flag on action). Ensure
RouterLink is imported/registered like other components (consistent with
SidebarNavItem) and apply the same change for the second occurrence that
iterates actions (lines referenced as 83-88).
In `@assets/vue/views/DashboardView.vue`:
- Around line 54-63: dashboardStats.chart currently accepts any truthy value and
passes it to child components; add light runtime shape guards in the component
before using it: validate that dashboardStats.chart is an object,
dashboardStats.chart.labels is an Array, and dashboardStats.chart.series is an
Array of objects where each series has a string name and an Array data; if any
check fails, fall back to the existing default chart object (labels: [], series:
[{name:'Opens', data:[]}, {name:'Clicks', data:[]}]) and similarly ensure
dashboardStats.recent_campaigns is an Array before assigning to recentCampaigns
(otherwise use []); update the code paths referencing dashboardStats.chart and
recentCampaigns to use these validated variables (e.g., validate around symbols
dashboardStats.chart, labels, series, recentCampaigns).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2da1f424-506d-48d5-bbd6-cbf741504065
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
assets/vue/components/dashboard/CampaignsTable.vueassets/vue/components/dashboard/KpiGrid.vueassets/vue/components/dashboard/PerformanceChartCard.vueassets/vue/components/dashboard/QuickActionsCard.vueassets/vue/components/dashboard/RecentCampaignsCard.vueassets/vue/views/DashboardView.vuepackage.jsonsrc/Controller/DashboardController.phpsrc/Security/SessionAuthenticator.phptemplates/spa.html.twig
✅ Files skipped from review due to trivial changes (2)
- templates/spa.html.twig
- assets/vue/components/dashboard/CampaignsTable.vue
🚧 Files skipped from review as they are similar to previous changes (5)
- assets/vue/components/dashboard/RecentCampaignsCard.vue
- assets/vue/components/dashboard/PerformanceChartCard.vue
- src/Controller/DashboardController.php
- assets/vue/components/dashboard/KpiGrid.vue
- package.json
Comment on lines
+85
to
+86
| { id: 'subscribers', label: 'Add Subscribers', href: '/subscribers' }, | ||
| { id: 'import', label: 'Import List', href: '/subscribers' }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Recon route candidates related to subscriber import flows.
rg -nP --type=js --type=ts --type=vue "path\\s*:\\s*['\"][^'\"]*(subscribers|import)[^'\"]*['\"]|['\"]/subscribers[^'\"]*['\"]"
rg -nP --type=php "Route\\(|/subscribers|import" src templatesRepository: phpList/web-frontend
Length of output: 1016
🏁 Script executed:
# Find and read the QuickActionsCard.vue file
find . -name "QuickActionsCard.vue" -type fRepository: phpList/web-frontend
Length of output: 118
🏁 Script executed:
# Search for import-related routes and paths
rg -in "import" --type=js --type=ts --type=php | grep -i "route\|path\|href" | head -20Repository: phpList/web-frontend
Length of output: 671
🏁 Script executed:
# Check for any import routes or list import routes
rg -in "import.*list\|list.*import" --type=php | head -20Repository: phpList/web-frontend
Length of output: 46
🏁 Script executed:
# Read QuickActionsCard.vue to see the context around lines 85-86
cat -n ./assets/vue/components/dashboard/QuickActionsCard.vue | head -100Repository: phpList/web-frontend
Length of output: 3981
🏁 Script executed:
# Read the router configuration to see all available routes
cat -n ./assets/router/index.jsRepository: phpList/web-frontend
Length of output: 921
🏁 Script executed:
# Search for any import-related views or components
fd -e vue | xargs grep -l "import\|Import" | grep -i "view\|list"Repository: phpList/web-frontend
Length of output: 135
Both Import List and Add Subscribers resolve to the same destination.
Lines 85–86 both point to /subscribers, despite being presented as distinct actions with different icons and labels. Currently, no dedicated import route exists in the router configuration. Either create a route for the import functionality or remove the duplicate action.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/dashboard/QuickActionsCard.vue` around lines 85 - 86,
QuickActionsCard.vue defines two actions with ids 'subscribers' and 'import'
that both point to '/subscribers', causing the duplicate destination; either add
a dedicated import route (e.g., create a route with path '/subscribers/import'
and a component like ImportSubscribersView or route name 'SubscribersImport' in
your router routes array) and change the 'import' action's href to
'/subscribers/import' (or its route name), or remove the 'import' action from
the QuickActionsCard.vue actions array to avoid the duplicate; update the
'import' item (id: 'import') and corresponding router entry (routes array /
route name) accordingly.
New Features
Mailing lists UI: directory, create/edit/delete modals, add-subscribers modal, per-list subscribers view with selection, pagination, copy/move/delete actions
Export UI for list subscribers with selectable columns and date-range options; export panel added to subscribers view
New frontend routes for Lists and List Subscribers
Refactor
CSV export now proxied from upstream export service
Chores
API client surface extended; runtime dependency bumped
Removed
Legacy subscribers table component removed
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/Service/SubscriberCollectionNormalizer.php (1)
22-24: Potential exception ifcreatedAtis malformed orsubscribedListsis null.
new DateTimeImmutable($subscriber->createdAt)will throw if the date string is invalid. Similarly,count($subscriber->subscribedLists)could fail if it's null. The API should always return valid data, but defensive handling might be worth considering for resilience.🛡️ Optional defensive handling
- 'createdAt' => (new DateTimeImmutable($subscriber->createdAt))->format('Y-m-d H:i:s'), + 'createdAt' => $subscriber->createdAt + ? (new DateTimeImmutable($subscriber->createdAt))->format('Y-m-d H:i:s') + : null, 'uniqueId' => $subscriber->uniqueId, - 'listCount' => count($subscriber->subscribedLists), + 'listCount' => count($subscriber->subscribedLists ?? []),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Service/SubscriberCollectionNormalizer.php` around lines 22 - 24, The code is vulnerable to exceptions when creating DateTimeImmutable from $subscriber->createdAt and when counting $subscriber->subscribedLists; update the normalization logic in SubscriberCollectionNormalizer (the block that builds 'createdAt'/'uniqueId'/'listCount') to defensively handle both: try to parse $subscriber->createdAt (use DateTimeImmutable::createFromFormat or a try/catch around new DateTimeImmutable) and if parsing fails set 'createdAt' to null or an ISO8601 fallback string, and ensure $subscriber->subscribedLists is coerced to an array (e.g. $lists = is_array($subscriber->subscribedLists) ? $subscriber->subscribedLists : []; then 'listCount' => count($lists)); keep 'uniqueId' unchanged.src/Controller/ListsController.php (1)
38-46: Unused$listIdparameter.The
$listIdis captured from the route but never used since the view just renders the SPA shell. This is intentional (client-side routing handles it), but you might want to suppress the warning explicitly.🔧 Optional: suppress unused parameter warning
#[Route('/{listId}/subscribers', name: 'list_subscribers', methods: ['GET'])] - public function view(Request $request, int $listId): JsonResponse|Response + public function view(Request $request, int $listId): JsonResponse|Response // phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameterOr if you prefer, use
#[SuppressWarnings(PHPMD.UnusedFormalParameter)]attribute.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/ListsController.php` around lines 38 - 46, The route handler ListsController::view captures a route parameter $listId but never uses it, causing an unused-parameter warning; to fix, explicitly acknowledge or suppress it by either renaming the parameter to _listId (or prefixing with an underscore) in the view method signature or add a suppression attribute such as #[SuppressWarnings(PHPMD.UnusedFormalParameter)] above the view method (or use the equivalent annotation your static analyzer expects) so the unused $listId warning is silenced while preserving the route behavior.src/Service/SubscriberExportRequestFactory.php (1)
20-23: Conflicting filter params silently resolve to one side.If someone passes both
confirmedandunconfirmed(or both blacklist variants),confirmedwins. This is probably fine since it's an edge case from malformed requests, but worth documenting or logging as unexpected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Service/SubscriberExportRequestFactory.php` around lines 20 - 23, The current logic in SubscriberExportRequestFactory for computing isConfirmed and isBlacklisted silently prefers one query param when both opposing flags are present; change the checks in the factory (the isConfirmed and isBlacklisted assignment logic) to detect the conflicting pair (e.g., both 'confirmed' and 'unconfirmed' or both 'blacklisted' and 'non-blacklisted') and treat that as an explicit error/invalid input: either return null/undefined for that filter and emit a warning via the request logger, or throw a validation exception (e.g., InvalidArgumentException) so callers know the input was malformed; update the assignments for isConfirmed and isBlacklisted to perform an explicit conflict check first, then the single-param checks, and add logging/exception handling accordingly.assets/router/index.js (1)
7-8: Consider passing a base path tocreateWebHistoryfor flexible deployments.The router currently assumes root-level deployment. If the app might be served from a sub-path (like
/admin/), pass the base as an argument:createWebHistory(import.meta.env.BASE_URL || '/'). Since phpList is typically deployed as a monolithic app, this may not be an immediate concern, but it's worth flagging for flexibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/router/index.js` around lines 7 - 8, The router is created with createRouter({ history: createWebHistory() }) which hardcodes root deployment; update the history creation in the router initialization (the createRouter call that sets history) to pass a base path from the environment such as createWebHistory(import.meta.env.BASE_URL || '/') so the app can run from a subpath (reference createWebHistory and router in the diff) — ensure the import.meta.env variable is used safely and falls back to '/'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/vue/components/lists/ListDirectory.vue`:
- Around line 95-116: The template currently renders two identical empty-state
rows (one with v-if="!isLoading && !loadError && mailingLists.length === 0" and
another with v-else-if="mailingLists.length === 0"), causing the message to
appear twice; remove one of these duplicate <tr> blocks so only a single
empty-state row remains (preferably keep the row that follows the isLoading /
loadError branches—i.e., the v-else-if="mailingLists.length === 0" block—to
preserve the conditional flow in ListDirectory.vue).
In `@assets/vue/components/lists/ListSubscribersExportPanel.vue`:
- Around line 145-152: The dynamic attribute columns added to
columnOptions.value are not being merged into the existing form.value.columns,
so async attributes get skipped on first export; after you extend
columnOptions.value with dynamicColumns in the component (the block that sets
columnOptions.value = [...columnOptions.value, ...dynamicColumns]), update
form.value.columns to include any newly added dynamic column values (e.g., the
values from dynamicColumns.map(c => c.value)) by taking the union of the
existing form.value.columns and the new dynamic values (preserving order or
using a Set to dedupe) so async attributes are selected even when
form.value.columns was already populated.
In `@assets/vue/components/subscribers/SubscriberDirectory.vue`:
- Around line 208-229: The fetch block currently calls response.json()
unconditionally which will throw or mis-handle non-OK or non-JSON responses;
before parsing, check response.ok (and keep the 401 redirect handling) and
validate Content-Type includes "application/json"; if not ok, read
response.text() for logging and set a visible failure state (e.g. clear or set
subscribers.value and pagination.value and/or set an error flag) so the
directory doesn't keep stale rows; ensure updateUrl(afterId) is only called on
successful JSON parse and use the existing subscribers.value, pagination.value,
and updateUrl symbols when updating state.
In `@assets/vue/views/ListSubscribersView.vue`:
- Around line 315-318: When the statusFilter changes we already reset
actionError and actionMessage but must also clear any current selection used by
bulk actions so invisible rows aren't affected; update the watcher on
statusFilter to also reset the component's selection state (e.g., clear selected
subscriber IDs / arrays such as selectedSubscribers, selectedSubscriberIds,
selectedRows or any bulkSelection variable used for delete/copy/move) and any
related bulk-action state (selection count, indeterminate flags) so bulk actions
only apply to visible items. Ensure you reference and reset the actual selection
variables used in this component alongside statusFilter, actionError, and
actionMessage.
In `@src/Controller/SubscribersController.php`:
- Around line 58-70: The subscribers_history array is global to the session so
pagination from one filter/search/tab bleeds into others; modify the logic
around request->getSession()->get('subscribers_history', []) and set(...) to
scope history by the active query signature (e.g. serialize or hash of
$request->query->all(), current tab, and relevant filters) instead of a single
'subscribers_history' key, then use that scoped history when pushing $afterId,
running array_search($afterId, $history, true) and computing $prevId so Previous
will be looked up only within the matching query-specific history.
---
Nitpick comments:
In `@assets/router/index.js`:
- Around line 7-8: The router is created with createRouter({ history:
createWebHistory() }) which hardcodes root deployment; update the history
creation in the router initialization (the createRouter call that sets history)
to pass a base path from the environment such as
createWebHistory(import.meta.env.BASE_URL || '/') so the app can run from a
subpath (reference createWebHistory and router in the diff) — ensure the
import.meta.env variable is used safely and falls back to '/'.
In `@src/Controller/ListsController.php`:
- Around line 38-46: The route handler ListsController::view captures a route
parameter $listId but never uses it, causing an unused-parameter warning; to
fix, explicitly acknowledge or suppress it by either renaming the parameter to
_listId (or prefixing with an underscore) in the view method signature or add a
suppression attribute such as #[SuppressWarnings(PHPMD.UnusedFormalParameter)]
above the view method (or use the equivalent annotation your static analyzer
expects) so the unused $listId warning is silenced while preserving the route
behavior.
In `@src/Service/SubscriberCollectionNormalizer.php`:
- Around line 22-24: The code is vulnerable to exceptions when creating
DateTimeImmutable from $subscriber->createdAt and when counting
$subscriber->subscribedLists; update the normalization logic in
SubscriberCollectionNormalizer (the block that builds
'createdAt'/'uniqueId'/'listCount') to defensively handle both: try to parse
$subscriber->createdAt (use DateTimeImmutable::createFromFormat or a try/catch
around new DateTimeImmutable) and if parsing fails set 'createdAt' to null or an
ISO8601 fallback string, and ensure $subscriber->subscribedLists is coerced to
an array (e.g. $lists = is_array($subscriber->subscribedLists) ?
$subscriber->subscribedLists : []; then 'listCount' => count($lists)); keep
'uniqueId' unchanged.
In `@src/Service/SubscriberExportRequestFactory.php`:
- Around line 20-23: The current logic in SubscriberExportRequestFactory for
computing isConfirmed and isBlacklisted silently prefers one query param when
both opposing flags are present; change the checks in the factory (the
isConfirmed and isBlacklisted assignment logic) to detect the conflicting pair
(e.g., both 'confirmed' and 'unconfirmed' or both 'blacklisted' and
'non-blacklisted') and treat that as an explicit error/invalid input: either
return null/undefined for that filter and emit a warning via the request logger,
or throw a validation exception (e.g., InvalidArgumentException) so callers know
the input was malformed; update the assignments for isConfirmed and
isBlacklisted to perform an explicit conflict check first, then the single-param
checks, and add logging/exception handling accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d67a461-c3d1-4f58-8636-eefd964fb79a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
assets/router/index.jsassets/vue/api.jsassets/vue/components/lists/AddSubscribersModal.vueassets/vue/components/lists/CreateListModal.vueassets/vue/components/lists/EditListModal.vueassets/vue/components/lists/ListDirectory.vueassets/vue/components/lists/ListSubscribersExportPanel.vueassets/vue/components/subscribers/SubscriberDirectory.vueassets/vue/views/ListSubscribersView.vueassets/vue/views/ListsView.vuepackage.jsonsrc/Controller/ListsController.phpsrc/Controller/SubscribersController.phpsrc/Service/SubscriberCollectionNormalizer.phpsrc/Service/SubscriberExportRequestFactory.php
✅ Files skipped from review due to trivial changes (1)
- package.json
Comment on lines
+95
to
+116
| <tr v-if="!isLoading && !loadError && mailingLists.length === 0"> | ||
| <td colspan="4" class="px-6 py-8 text-center text-slate-500"> | ||
| No mailing lists found. | ||
| </td> | ||
| </tr> | ||
| <tr v-if="isLoading"> | ||
| <td colspan="4" class="px-6 py-8 text-center text-slate-500"> | ||
| Loading mailing lists... | ||
| </td> | ||
| </tr> | ||
|
|
||
| <tr v-else-if="loadError"> | ||
| <td colspan="4" class="px-6 py-8 text-center text-red-600"> | ||
| {{ loadError }} | ||
| </td> | ||
| </tr> | ||
|
|
||
| <tr v-else-if="mailingLists.length === 0"> | ||
| <td colspan="4" class="px-6 py-8 text-center text-slate-500"> | ||
| No mailing lists found. | ||
| </td> | ||
| </tr> |
There was a problem hiding this comment.
Remove the duplicated empty-state branch.
When the fetch succeeds with zero lists, both empty-state rows render, so the desktop table shows the same “No mailing lists found.” message twice.
♻️ Proposed fix
- <tr v-if="!isLoading && !loadError && mailingLists.length === 0">
- <td colspan="4" class="px-6 py-8 text-center text-slate-500">
- No mailing lists found.
- </td>
- </tr>
<tr v-if="isLoading">
<td colspan="4" class="px-6 py-8 text-center text-slate-500">
Loading mailing lists...
</td>
</tr>📝 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
| <tr v-if="!isLoading && !loadError && mailingLists.length === 0"> | |
| <td colspan="4" class="px-6 py-8 text-center text-slate-500"> | |
| No mailing lists found. | |
| </td> | |
| </tr> | |
| <tr v-if="isLoading"> | |
| <td colspan="4" class="px-6 py-8 text-center text-slate-500"> | |
| Loading mailing lists... | |
| </td> | |
| </tr> | |
| <tr v-else-if="loadError"> | |
| <td colspan="4" class="px-6 py-8 text-center text-red-600"> | |
| {{ loadError }} | |
| </td> | |
| </tr> | |
| <tr v-else-if="mailingLists.length === 0"> | |
| <td colspan="4" class="px-6 py-8 text-center text-slate-500"> | |
| No mailing lists found. | |
| </td> | |
| </tr> | |
| <tr v-if="isLoading"> | |
| <td colspan="4" class="px-6 py-8 text-center text-slate-500"> | |
| Loading mailing lists... | |
| </td> | |
| </tr> | |
| <tr v-else-if="loadError"> | |
| <td colspan="4" class="px-6 py-8 text-center text-red-600"> | |
| {{ loadError }} | |
| </td> | |
| </tr> | |
| <tr v-else-if="mailingLists.length === 0"> | |
| <td colspan="4" class="px-6 py-8 text-center text-slate-500"> | |
| No mailing lists found. | |
| </td> | |
| </tr> |
🤖 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 95 - 116, The
template currently renders two identical empty-state rows (one with
v-if="!isLoading && !loadError && mailingLists.length === 0" and another with
v-else-if="mailingLists.length === 0"), causing the message to appear twice;
remove one of these duplicate <tr> blocks so only a single empty-state row
remains (preferably keep the row that follows the isLoading / loadError
branches—i.e., the v-else-if="mailingLists.length === 0" block—to preserve the
conditional flow in ListDirectory.vue).
Comment on lines
+145
to
+152
| columnOptions.value = [ | ||
| ...columnOptions.value, | ||
| ...dynamicColumns | ||
| ] | ||
|
|
||
| if (!form.value.columns?.length) { | ||
| form.value.columns = columnOptions.value.map(c => c.value) | ||
| } |
There was a problem hiding this comment.
Async attribute columns are excluded from the initial export.
form.columns is already non-empty when the attribute fetch completes, so these new columns never join the default selection. A first export from this panel will silently skip all custom attributes unless the user notices and re-checks them.
♻️ Proposed fix
- columnOptions.value = [
- ...columnOptions.value,
- ...dynamicColumns
- ]
-
- if (!form.value.columns?.length) {
- form.value.columns = columnOptions.value.map(c => c.value)
- }
+ const hadAllColumnsSelected =
+ form.value.columns.length === columnOptions.value.length
+
+ columnOptions.value = [
+ ...columnOptions.value,
+ ...dynamicColumns
+ ]
+
+ if (hadAllColumnsSelected) {
+ form.value.columns = columnOptions.value.map((c) => c.value)
+ }📝 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
| columnOptions.value = [ | |
| ...columnOptions.value, | |
| ...dynamicColumns | |
| ] | |
| if (!form.value.columns?.length) { | |
| form.value.columns = columnOptions.value.map(c => c.value) | |
| } | |
| const hadAllColumnsSelected = | |
| form.value.columns.length === columnOptions.value.length | |
| columnOptions.value = [ | |
| ...columnOptions.value, | |
| ...dynamicColumns | |
| ] | |
| if (hadAllColumnsSelected) { | |
| form.value.columns = columnOptions.value.map((c) => c.value) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/lists/ListSubscribersExportPanel.vue` around lines 145
- 152, The dynamic attribute columns added to columnOptions.value are not being
merged into the existing form.value.columns, so async attributes get skipped on
first export; after you extend columnOptions.value with dynamicColumns in the
component (the block that sets columnOptions.value = [...columnOptions.value,
...dynamicColumns]), update form.value.columns to include any newly added
dynamic column values (e.g., the values from dynamicColumns.map(c => c.value))
by taking the union of the existing form.value.columns and the new dynamic
values (preserving order or using a Set to dedupe) so async attributes are
selected even when form.value.columns was already populated.
Comment on lines
+208
to
+229
| try { | ||
| const response = await fetch(url, { | ||
| headers: { Accept: 'application/json', 'X-Requested-With': 'XMLHttpRequest' }, | ||
| signal | ||
| }) | ||
|
|
||
| if (response.status === 401) { | ||
| const data = await response.json() | ||
| window.location.href = data.redirect | ||
| return | ||
| } | ||
|
|
||
| const data = await response.json() | ||
|
|
||
| subscribers.value = data.items | ||
| pagination.value = data.pagination | ||
|
|
||
| updateUrl(afterId) | ||
| } catch (error) { | ||
| if (error?.name === 'AbortError') return | ||
| console.error('Failed to fetch subscribers:', error) | ||
| } |
There was a problem hiding this comment.
Guard non-OK and non-JSON responses before parsing.
This path goes straight to response.json(). If the backend returns a 5xx HTML page or an auth page, the catch only logs it and the directory keeps showing stale rows with no visible failure state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/subscribers/SubscriberDirectory.vue` around lines 208 -
229, The fetch block currently calls response.json() unconditionally which will
throw or mis-handle non-OK or non-JSON responses; before parsing, check
response.ok (and keep the 401 redirect handling) and validate Content-Type
includes "application/json"; if not ok, read response.text() for logging and set
a visible failure state (e.g. clear or set subscribers.value and
pagination.value and/or set an error flag) so the directory doesn't keep stale
rows; ensure updateUrl(afterId) is only called on successful JSON parse and use
the existing subscribers.value, pagination.value, and updateUrl symbols when
updating state.
Comment on lines
+315
to
+318
| watch(statusFilter, () => { | ||
| actionError.value = '' | ||
| actionMessage.value = '' | ||
| }) |
There was a problem hiding this comment.
Clear selection when the status filter changes.
Bulk actions currently keep acting on subscribers selected under the previous filter, even though those rows may no longer be visible. That makes delete/copy/move easy to apply to hidden items by mistake.
♻️ Proposed fix
watch(statusFilter, () => {
actionError.value = ''
actionMessage.value = ''
+ clearSelection()
})📝 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
| watch(statusFilter, () => { | |
| actionError.value = '' | |
| actionMessage.value = '' | |
| }) | |
| watch(statusFilter, () => { | |
| actionError.value = '' | |
| actionMessage.value = '' | |
| clearSelection() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/views/ListSubscribersView.vue` around lines 315 - 318, When the
statusFilter changes we already reset actionError and actionMessage but must
also clear any current selection used by bulk actions so invisible rows aren't
affected; update the watcher on statusFilter to also reset the component's
selection state (e.g., clear selected subscriber IDs / arrays such as
selectedSubscribers, selectedSubscriberIds, selectedRows or any bulkSelection
variable used for delete/copy/move) and any related bulk-action state (selection
count, indeterminate flags) so bulk actions only apply to visible items. Ensure
you reference and reset the actual selection variables used in this component
alongside statusFilter, actionError, and actionMessage.
Comment on lines
+58
to
+70
| $history = $request->getSession()->get('subscribers_history', []); | ||
| if (!in_array($afterId, $history, true)) { | ||
| $history[] = $afterId; | ||
| $request->getSession()->set('subscribers_history', $history); | ||
| } | ||
|
|
||
| $prevId = null; | ||
| $index = array_search($afterId, $history, true); | ||
| if ($index === 0) { | ||
| $prevId = 0; | ||
| } elseif ($index > 0) { | ||
| $prevId = $history[$index - 1]; | ||
| } |
There was a problem hiding this comment.
Key pagination history by the active query, not the whole session.
This single subscribers_history array is shared across filters, searches, and tabs. After paging in one result set and then changing filters, prevId can come from the wrong query and send “Previous” to the wrong page.
♻️ Proposed fix
- $history = $request->getSession()->get('subscribers_history', []);
+ $historyKey = 'subscribers_history_' . md5((string) json_encode([
+ 'confirmed' => $request->query->has('confirmed'),
+ 'unconfirmed' => $request->query->has('unconfirmed'),
+ 'blacklisted' => $request->query->has('blacklisted'),
+ 'non_blacklisted' => $request->query->has('non-blacklisted'),
+ 'findColumn' => $request->query->get('findColumn'),
+ 'findValue' => $request->query->get('findValue'),
+ ]));
+ $history = $request->getSession()->get($historyKey, []);
if (!in_array($afterId, $history, true)) {
$history[] = $afterId;
- $request->getSession()->set('subscribers_history', $history);
+ $request->getSession()->set($historyKey, $history);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Controller/SubscribersController.php` around lines 58 - 70, The
subscribers_history array is global to the session so pagination from one
filter/search/tab bleeds into others; modify the logic around
request->getSession()->get('subscribers_history', []) and set(...) to scope
history by the active query signature (e.g. serialize or hash of
$request->query->all(), current tab, and relevant filters) instead of a single
'subscribers_history' key, then use that scoped history when pushing $afterId,
running array_search($afterId, $history, true) and computing $prevId so Previous
will be looked up only within the matching query-specific history.
New Features
Campaigns area: directory with filtering, pagination and per-campaign actions (View, Edit, Delete, Suspend, Requeue, Copy to draft)
Create/Edit campaign pages and routes; multi-step edit workflow with templates, lists, scheduling, test send and save flows
Campaign details modal with preview and “Resend to lists”
Topbar global search across subscribers and campaigns
Style / Layout
Sidebar responsiveness updated for improved desktop behavior
Frontend asset loading changed to explicit asset package and deferred scripts; favicon served via frontend asset package
Chores
Expanded REST API client surface and added a paginated fetch helper for lists
Added CKEditor rich-text editor integration
Documentation
Installation guidance for publishing bundle assets to the host app public path
---------
Co-authored-by: Tatevik <tatevikg1@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (4)
src/Security/SessionAuthenticator.php (2)
48-54:⚠️ Potential issue | 🟠 MajorValidate the session token before granting admin access.
Any session containing
auth_tokencurrently receivesROLE_ADMIN. At minimum, enforce the storedauth_expiry_date; preferably validate/introspect the token before creating the passport.🛡️ Minimal expiry-check direction
- if ($session->has('auth_token')) { + $authToken = (string) $session->get('auth_token', ''); + $expiryDate = $session->get('auth_expiry_date'); + + if ($authToken !== '' && $this->isTokenNotExpired($expiryDate)) { // Build a simple user granted ROLE_ADMIN when a session token exists $userBadge = new UserBadge('session-user', function (): InMemoryUser { return new InMemoryUser('session-user', '', ['ROLE_ADMIN']); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Security/SessionAuthenticator.php` around lines 48 - 54, The current authenticate flow in SessionAuthenticator grants ROLE_ADMIN whenever $session->has('auth_token'); change it to validate the stored token before creating the UserBadge/SelfValidatingPassport: retrieve $session->get('auth_token') and $session->get('auth_expiry_date'), check expiry (and if available perform token introspection/validation), and only then construct the UserBadge('session-user', ...) with ['ROLE_ADMIN'] and return the SelfValidatingPassport; if validation/expiry check fails, do not create the admin badge and instead fall through to the unauthenticated path or throw an AuthenticationException. Ensure you reference the existing UserBadge and SelfValidatingPassport creation points when adding these checks.
26-33:⚠️ Potential issue | 🟠 MajorKeep public/debug paths out of the authenticator.
This still authenticates paths such as
/_profiler,/_wdt, and/build/when they reach Symfony, which can break debug tools or asset loading behind the catch-all firewall.🔧 Proposed fix
$path = $this->normalizePath($request->getPathInfo()); - if ($path === '/login' || str_starts_with($path, '/login/')) { - return false; + foreach (['/login', '/_profiler', '/_wdt', '/build/', '/favicon.ico', '/robots.txt'] as $publicPath) { + if ($path === $publicPath || str_starts_with($path, rtrim($publicPath, '/') . '/')) { + return false; + } } return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Security/SessionAuthenticator.php` around lines 26 - 33, The supports(Request $request) method in SessionAuthenticator is too permissive and still runs for debug/public asset routes; update supports (which calls normalizePath) to return false for common Symfony debug and public prefixes such as '/_profiler', '/_wdt', '/_debug', '/_error' and '/build/' (and any other project-specific public prefixes) by checking equality and str_starts_with for each prefix before returning true so profiler, web debug toolbar and asset requests bypass the authenticator.assets/vue/components/lists/ListDirectory.vue (1)
95-116:⚠️ Potential issue | 🟡 MinorRemove the duplicated desktop empty-state row.
When the fetch succeeds with zero lists, Lines 95-99 and Lines 112-116 both render “No mailing lists found.”
Proposed cleanup
- <tr v-if="!isLoading && !loadError && mailingLists.length === 0"> - <td colspan="4" class="px-6 py-8 text-center text-slate-500"> - No mailing lists found. - </td> - </tr> <tr v-if="isLoading"> <td colspan="4" class="px-6 py-8 text-center text-slate-500"> Loading mailing lists...🤖 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 95 - 116, The template currently renders the "No mailing lists found." empty-state twice; remove the duplicate <tr> block so there is only one empty-state row rendered when isLoading is false, loadError is falsy, and mailingLists.length === 0; update the conditional logic around the <tr> elements that use isLoading, loadError, and mailingLists.length (in ListDirectory.vue) so the order remains: show loading when isLoading, show error when loadError, otherwise show a single "No mailing lists found." row when mailingLists.length === 0.src/Controller/SubscribersController.php (1)
58-70:⚠️ Potential issue | 🟠 MajorScope subscriber pagination history to the active query.
subscribers_historyis shared across filters/searches, soprevIdcan come from a different result set and send “Previous” to the wrong page.Proposed direction
- $history = $request->getSession()->get('subscribers_history', []); + $historyKey = 'subscribers_history_' . md5((string) json_encode([ + 'confirmed' => $request->query->has('confirmed'), + 'unconfirmed' => $request->query->has('unconfirmed'), + 'blacklisted' => $request->query->has('blacklisted'), + 'non_blacklisted' => $request->query->has('non-blacklisted'), + 'findColumn' => $request->query->get('findColumn'), + 'findValue' => $request->query->get('findValue'), + ])); + $history = $request->getSession()->get($historyKey, []); if (!in_array($afterId, $history, true)) { $history[] = $afterId; - $request->getSession()->set('subscribers_history', $history); + $request->getSession()->set($historyKey, $history); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/SubscribersController.php` around lines 58 - 70, The subscribers_history is global across different searches causing prevId to come from the wrong result set; scope history to the active query by deriving a query-specific key (e.g., hash the current filter/search params from the Request) and store per-query histories instead of the single 'subscribers_history' entry. Replace uses of $request->getSession()->get('subscribers_history', []) and ->set('subscribers_history', $history) with get/set using an associative map keyed by the query key (e.g., $queryKey => $history) and then perform array_search($afterId, $history, true) and compute $prevId against that scoped $history; ensure the same queryKey logic is applied wherever subscribers_history is read or written.
🧹 Nitpick comments (7)
src/DependencyInjection/Compiler/RegisterEndpointsPass.php (3)
35-53: Filename-to-FQCN mapping misses sub-namespaces and abstract-but-useful base classes.Two edge cases to consider:
basename($endpointFile, '.php')flattens the namespace, so if the client package ever grows a subdirectory (e.g.,Endpoint/Admin/FooEndpoint.php), the glob pattern won't find it and the FQCN would be wrong even if it did. If the directory is guaranteed flat today, fine — just know that adding a subdir silently breaks discovery.RecursiveDirectoryIterator+ trimming the prefix is a one-liner upgrade when/if needed.isInstantiable()skips abstracts and interfaces (good), but it also skips classes with a non-public constructor. If the client package ever adds a protected-ctor endpoint intended to be built via a factory, it'll silently be skipped with no log. Adebug-level log on skip would make future debugging much less painful.Totally chill as-is for the current flat layout — flagging for future-you.
🤖 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 35 - 53, The current filename-to-FQCN logic flattens sub-namespaces and silently skips classes with non-public constructors; update the discovery in RegisterEndpointsPass to use a recursive file iterator (e.g., RecursiveDirectoryIterator/RecursiveIteratorIterator) and compute the FQCN by trimming the base Endpoint directory prefix and replacing directory separators with namespace separators instead of using basename($endpointFile, '.php'), and keep the existing class_exists/ReflectionClass checks but add a debug-level log whenever you skip a class (e.g., when !class_exists($className), when ReflectionClass->isInstantiable() is false, or when the constructor is non-public) so skips are observable; continue to respect container->hasDefinition / container->hasAlias as before.
20-28: Add a clarifying comment about the custom kernel.application_dir parameter, and consider a more robust path separator approach for Windows compatibility.The defensive fallback pattern is clean, but two small refinements:
kernel.application_dirisn't a Symfony standard parameter—add a brief inline comment explaining why this custom parameter is checked (e.g., "custom param for app-specific root directory").- The
rtrim($projectDir, '/')call only strips forward slashes. While Symfony normalizes paths cross-platform, usingrtrim($projectDir, '/\\')orDIRECTORY_SEPARATORis more defensive against edge cases on Windows where mixed separators could appear.Both are polishing touches, not blockers.
🤖 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 20 - 28, Add a brief inline comment explaining that the non-standard parameter kernel.application_dir is a custom app-specific root directory fallback (so reviewers understand why it’s checked) and make the path trimming defensive for Windows by changing the rtrim call on $projectDir to strip both forward and backslashes (e.g., rtrim($projectDir, '/\\') or otherwise normalize using DIRECTORY_SEPARATOR) before building $endpointDirectory; keep the existing fallback logic intact and update the comment near $projectDir, rtrim($projectDir, ...), and $endpointDirectory to reflect the change.
28-33: The vendor-path scanning here is fragile — consider a Composer-native approach instead.Hardcoding
vendor/tatevikgr/rest-api-client/src/Endpointcouples this bundle to a specific vendor layout. While the code gracefully returns early if the dir is missing, the approach itself has structural issues:
- Custom
vendor-dirincomposer.jsonwould break this.- Path/symlink repositories during local development of the client would fail.
- A package rename (e.g.,
phplist/rest-api-client) requires manual string updates.- It assumes a specific internal structure (
src/Endpoint) that the package might reorganize.A more resilient approach: ask Composer where the package lives, or iterate PSR-4 namespaces directly from the autoloader. Even better: if the client package provides its own service configuration, loading via
Resource::fromservices.yaml(or a bundle inside the client registering its own endpoints) removes this pass entirely.♻️ Sketch: resolve via Composer's ClassLoader
$prefixes = \Composer\Autoload\ClassLoader::class ? (require $projectDir . '/vendor/autoload.php')->getPrefixesPsr4() : []; $roots = $prefixes['PhpList\\RestApiClient\\Endpoint\\'] ?? []; foreach ($roots as $endpointDirectory) { if (!is_dir($endpointDirectory)) { continue; } // ...existing glob + register loop... }🤖 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 28 - 33, Replace the fragile hardcoded vendor path logic in RegisterEndpointsPass that builds $endpointDirectory and uses glob on $endpointFiles with a Composer-aware discovery: load the project's vendor/autoload.php, call getPrefixesPsr4() on the Composer\Autoload\ClassLoader, find the PSR-4 prefix used by the REST client (e.g. the Endpoint namespace used by the client), iterate each mapped directory root returned for that prefix, skip non-directories, and run the existing file-globbing/registration logic inside that loop; this removes assumptions about vendor-dir, package name, and internal layout and reuses the autoloader to locate the true package paths.src/DependencyInjection/PhpListFrontendExtension.php (1)
58-82: Remove the inactive security configuration block.This commented block is substantial and can drift from
config/packages/security.yaml. Keeping only the active config reduces confusion.🧹 Proposed cleanup
- -// $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'], -// ], -// ]);🤖 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, Remove the large commented-out security configuration block that begins with $container->prependExtensionConfig('security', ...) inside the PhpListFrontendExtension class; delete the entire commented segment so only the active configuration remains, keeping the file focused and preventing drift from config/packages/security.yaml.assets/vue/components/campaigns/ViewCampaignModal.vue (1)
2-16: Add dialog semantics and keyboard close support.The modal is visually clear, but screen readers do not get dialog context, and Escape does not close it. Add
role,aria-modal, a label, and an Escape handler.♿ Proposed improvement
<div v-if="isViewModalOpen" + role="dialog" + aria-modal="true" + aria-labelledby="campaign-details-title" class="fixed inset-0 z-50 flex items-center justify-center bg-slate-900/50 p-4" `@click.self`="emit('close')" + `@keydown.esc`="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-details-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 - 16, The modal container (the div with v-if="isViewModalOpen") needs dialog semantics and Escape-key support: add role="dialog" and aria-modal="true" and set aria-labelledby to the header by giving the h3 a stable id (e.g. id="campaign-title"), add tabindex="-1" to the modal container so it can receive focus, and wire Escape to close (e.g. `@keydown.esc`="emit('close')" on the container or a small onEscape handler). Also add a ref (e.g. modalRef) and a watcher on isViewModalOpen to focus modalRef when it opens so screen readers land in the dialog; keep the existing `@click.self`="emit('close')" and the h3 text (campaign?.id) intact..github/workflows/ci.yml (1)
92-96: Pin Prism to devDependencies instead of installing globally.
npm install -g@stoplight/prism-cli`` can break when new releases change behavior. Add Prism todevDependenciesin `package.json` and invoke the local binary from `node_modules/.bin/` instead.🔧 Suggested workflow update
- - name: Install Prism - run: npm install -g `@stoplight/prism-cli` + - name: Verify Prism + run: ./node_modules/.bin/prism --version - name: Start Prism Mock Server run: | - prism mock --host 127.0.0.1 --port 4010 ./openapi.json & + ./node_modules/.bin/prism mock --host 127.0.0.1 --port 4010 ./openapi.json &🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 92 - 96, Replace the global install step and direct prism invocation with a devDependency and local binary invocation: add "@stoplight/prism-cli" to package.json devDependencies (so the project installs Prism via npm ci) and update the workflow steps that reference the "Install Prism" and "Start Prism Mock Server" tasks to run the local binary (e.g., node_modules/.bin/prism or npx prism) instead of npm install -g and the direct "prism mock ..." call; ensure the step that starts the mock server still uses the same host/port/flags and backgrounds the process as before.tests/Unit/Service/SubscriberCollectionNormalizerTest.php (1)
91-108: Rename or remove the “null items” test.This does not exercise null items because
SubscriberCollection::$itemsis typed as an array, so it duplicates the empty collection case and may mislead future maintainers.Small cleanup option
- public function testNormalizeNullItems(): void + public function testNormalizeEmptyItemsRemainArray(): void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Service/SubscriberCollectionNormalizerTest.php` around lines 91 - 108, The test testNormalizeNullItems in SubscriberCollectionNormalizerTest is misleading because SubscriberCollection::$items is typed as array and the test only covers an empty items case; either remove this test or rename it to reflect the actual behavior (e.g., testNormalizeEmptyItems) and update its comment and assertions accordingly so it no longer claims to exercise null items; locate the test method testNormalizeNullItems and either delete it or rename it and adjust the inline comment that mentions null to accurately describe the empty-array scenario, leaving the call to $this->normalizer->normalize($collection, null, null) and the assertions intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 61: Replace the interactive apt install invocation in the CI workflow:
change the line that runs "sudo apt install socat" to a non-interactive
installation using apt-get with the -y flag and include an apt-get update
beforehand (e.g., use apt-get update && apt-get install -y socat) so the CI
won't prompt for confirmation.
In `@assets/router/index.js`:
- Around line 17-27: The router currently overwrites dynamic titles because
meta.title is a static string for the route '/campaigns/:campaignId/edit' (name:
'campaign-edit') and router.afterEach always uses to.meta.title; change
meta.title to accept either a string or a function and update the
router.afterEach hook (router.afterEach) to detect if to.meta.title is a
function and, if so, call it with the current route (to) to get the pageTitle
(falling back to the string value or defaultTitle) so CampaignEditView can
surface the computed "Edit Campaign `#123`" title without losing support for
static titles.
In `@assets/vue/api.js`:
- Around line 40-53: The loop stops pagination incorrectly because
Number.isFinite(nextCursor) assumes numeric cursors; replace that condition with
a null/undefined check to allow opaque (string/UUID) cursors: change the break
condition to use nextCursor == null (or nextCursor === undefined || nextCursor
=== null) and keep the existing nextCursor === afterId check; reference the
variables and call: listClient.getLists, nextCursor, hasMore, afterId. Also add
a debug/info log when the loop exits because it reached the maxPages limit to
avoid silent truncation.
In `@assets/vue/components/base/BaseBadge.spec.js`:
- Around line 28-30: The failing test uses the utility class text-ext-wf3 that
is not preserved by Tailwind; update tailwind.config.js so this color is never
purged by either adding the color token to theme.extend.colors (e.g., ext-wf3)
or adding the specific utilities 'text-ext-wf3' and 'bg-ext-wf3' to the
safelist; modify the config accordingly so components like BaseBadge.vue (and
tests in BaseBadge.spec.js) can use text-ext-wf3 reliably in production.
In `@assets/vue/components/base/CkEditorField.vue`:
- Around line 3-16: The CKEditor editable root isn't receiving the visible
label, so screen readers see a generic name; update the component to inject the
visible label into the editor configuration by ensuring editorConfig.root.label
is set to the component's label (e.g., inside CkEditorField.vue merge or assign
{ root: { label: label } } into the existing editorConfig before passing it to
the <ckeditor> component), referencing fieldId and ClassicEditor as before so
the editable area receives the aria-label matching the visible label.
In `@assets/vue/components/campaigns/CampaignDirectory.vue`:
- Around line 767-785: The pagination loop in fetchAllCampaigns can spin if the
API returns the same cursor repeatedly; after computing nextCursor
(response?.pagination?.nextCursor) compare it to the current cursor and break
the loop if nextCursor === cursor to avoid refetching the same page and
duplicating items; keep the existing guard and push logic unchanged but add this
explicit cursor-advance check using the local variables cursor and nextCursor in
fetchAllCampaigns, and apply the same nextCursor === cursor guard to the other
statistics pagination loops referenced (where they use cursor/nextCursor and
guard).
In `@assets/vue/components/campaigns/ViewCampaignModal.vue`:
- Around line 177-184: handleResend currently maps selectedMailingListIds.value
to Numbers and filters invalids, silently dropping bad IDs; instead, after
creating listIds (from selectedMailingListIds.value.map(...)), compare
listIds.length to selectedMailingListIds.value.length and if they differ, do not
emit('resend', ...); instead surface a validation error (e.g. emit a specific
event like emit('validation-error', { message: 'One or more selected mailing
list IDs are invalid' }) or set a component error state) so the UI can inform
the user; if lengths match and props.campaign?.id exists, then proceed to
emit('resend', listIds).
In `@assets/vue/views/CampaignEditView.vue`:
- Around line 464-492: The warnings computed property doesn't include the
missing confirmation-token condition used by canQueueCampaign; update the
warnings computed (the warnings constant) to also check hasConfirmationUrlToken
(or replicate its logic) and push a user-facing message like "A confirmation
token ([CONFIRMATIONURL]) is required for this send format (or set Send format
to 'invite')" when the token is absent so the warning panel explains why
canQueueCampaign is disabled; reference the warnings computed, the
hasConfirmationUrlToken computed, and canQueueCampaign to locate and implement
the change.
In `@config/packages/security.yaml`:
- Around line 22-23: The YAML entries for the security firewalls contain
unquoted regex paths that break parsing (the lines with { path:
^/(?:app(?:_test)?\.php/)?login(?:/|$), roles: PUBLIC_ACCESS } and { path:
^/(?:app(?:_test)?\.php/)?api/v2(?:/|$), roles: PUBLIC_ACCESS }); fix by
wrapping each regex path value in quotes (e.g. path: "^(?:...)" ) so the
non-capturing group syntax (?:) is treated as a string by the YAML parser and
the file parses correctly; update the two entries referencing the login and
api/v2 path patterns accordingly.
In `@src/Controller/CampaignsController.php`:
- Around line 35-36: The route for the edit action currently allows non-numeric
segments so requests like /campaigns/not-a-number/edit reach
CampaignsController::edit(Request $request, int $campaignId): Response and fail
during parameter coercion; add a route requirement to constrain {campaignId} to
digits (e.g. requirement for "\d+") on the #[Route(...)] attribute for the edit
method to return a 404 for non-numeric IDs; apply the same change to
ListsController where the route uses {listId} to constrain that parameter to
numeric values as well.
In `@src/Controller/ListsController.php`:
- Around line 38-39: The route attribute on the view method is missing a
requirement for the listId placeholder so non-numeric paths reach the
controller; update the #[Route(...)] attribute on the view(Request $request, int
$listId) method in ListsController to include a requirements entry that
constrains listId to digits only (e.g. using a \d+ or [0-9]+ pattern) so invalid
slugs return a 404 before the controller is invoked.
In `@tests/Integration/Controller/AuthControllerTest.php`:
- Line 25: Remove the unconditional screenshot write in AuthControllerTest (the
$client->takeScreenshot call) and instead either delete the call or guard it
behind a screenshot-enabled check (e.g., read an environment flag like
SCREENSHOTS_ENABLED) so the test only attempts $client->takeScreenshot when
screenshots are explicitly enabled; apply the same change to
DashboardControllerPantherTest where the same pattern appears (the
$client->takeScreenshot invocation) and if you keep the feature, ensure the
directory is created only when the flag is set.
In `@tests/Integration/Controller/DashboardControllerPantherTest.php`:
- Line 31: The test calls
$client->takeScreenshot('var/screenshots/login-page.png') but never ensures the
var/screenshots directory exists; update the DashboardControllerPantherTest
(where takeScreenshot is invoked) to create the directory before calling
takeScreenshot (e.g., using mkdir or Symfony Filesystem) so that var/screenshots
is created with correct permissions when missing; ensure this creation happens
just prior to the takeScreenshot call in the test setup or the test method so
the screenshot write cannot fail.
In `@tests/Integration/Controller/ListsControllerPantherTest.php`:
- Around line 26-30: The test may inherit cookies from a reused Panther browser
causing an existing auth_token to bypass the anonymous redirect; before calling
$client->request('GET', $path) clear the browser cookie jar by invoking
$client->getCookieJar()->clear() (i.e., call getCookieJar()->clear() on the
$client returned by createPantherClient(...) immediately after creation) so the
request runs in a clean anonymous state.
---
Duplicate comments:
In `@assets/vue/components/lists/ListDirectory.vue`:
- Around line 95-116: The template currently renders the "No mailing lists
found." empty-state twice; remove the duplicate <tr> block so there is only one
empty-state row rendered when isLoading is false, loadError is falsy, and
mailingLists.length === 0; update the conditional logic around the <tr> elements
that use isLoading, loadError, and mailingLists.length (in ListDirectory.vue) so
the order remains: show loading when isLoading, show error when loadError,
otherwise show a single "No mailing lists found." row when mailingLists.length
=== 0.
In `@src/Controller/SubscribersController.php`:
- Around line 58-70: The subscribers_history is global across different searches
causing prevId to come from the wrong result set; scope history to the active
query by deriving a query-specific key (e.g., hash the current filter/search
params from the Request) and store per-query histories instead of the single
'subscribers_history' entry. Replace uses of
$request->getSession()->get('subscribers_history', []) and
->set('subscribers_history', $history) with get/set using an associative map
keyed by the query key (e.g., $queryKey => $history) and then perform
array_search($afterId, $history, true) and compute $prevId against that scoped
$history; ensure the same queryKey logic is applied wherever subscribers_history
is read or written.
In `@src/Security/SessionAuthenticator.php`:
- Around line 48-54: The current authenticate flow in SessionAuthenticator
grants ROLE_ADMIN whenever $session->has('auth_token'); change it to validate
the stored token before creating the UserBadge/SelfValidatingPassport: retrieve
$session->get('auth_token') and $session->get('auth_expiry_date'), check expiry
(and if available perform token introspection/validation), and only then
construct the UserBadge('session-user', ...) with ['ROLE_ADMIN'] and return the
SelfValidatingPassport; if validation/expiry check fails, do not create the
admin badge and instead fall through to the unauthenticated path or throw an
AuthenticationException. Ensure you reference the existing UserBadge and
SelfValidatingPassport creation points when adding these checks.
- Around line 26-33: The supports(Request $request) method in
SessionAuthenticator is too permissive and still runs for debug/public asset
routes; update supports (which calls normalizePath) to return false for common
Symfony debug and public prefixes such as '/_profiler', '/_wdt', '/_debug',
'/_error' and '/build/' (and any other project-specific public prefixes) by
checking equality and str_starts_with for each prefix before returning true so
profiler, web debug toolbar and asset requests bypass the authenticator.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 92-96: Replace the global install step and direct prism invocation
with a devDependency and local binary invocation: add "@stoplight/prism-cli" to
package.json devDependencies (so the project installs Prism via npm ci) and
update the workflow steps that reference the "Install Prism" and "Start Prism
Mock Server" tasks to run the local binary (e.g., node_modules/.bin/prism or npx
prism) instead of npm install -g and the direct "prism mock ..." call; ensure
the step that starts the mock server still uses the same host/port/flags and
backgrounds the process as before.
In `@assets/vue/components/campaigns/ViewCampaignModal.vue`:
- Around line 2-16: The modal container (the div with v-if="isViewModalOpen")
needs dialog semantics and Escape-key support: add role="dialog" and
aria-modal="true" and set aria-labelledby to the header by giving the h3 a
stable id (e.g. id="campaign-title"), add tabindex="-1" to the modal container
so it can receive focus, and wire Escape to close (e.g.
`@keydown.esc`="emit('close')" on the container or a small onEscape handler). Also
add a ref (e.g. modalRef) and a watcher on isViewModalOpen to focus modalRef
when it opens so screen readers land in the dialog; keep the existing
`@click.self`="emit('close')" and the h3 text (campaign?.id) intact.
In `@src/DependencyInjection/Compiler/RegisterEndpointsPass.php`:
- Around line 35-53: The current filename-to-FQCN logic flattens sub-namespaces
and silently skips classes with non-public constructors; update the discovery in
RegisterEndpointsPass to use a recursive file iterator (e.g.,
RecursiveDirectoryIterator/RecursiveIteratorIterator) and compute the FQCN by
trimming the base Endpoint directory prefix and replacing directory separators
with namespace separators instead of using basename($endpointFile, '.php'), and
keep the existing class_exists/ReflectionClass checks but add a debug-level log
whenever you skip a class (e.g., when !class_exists($className), when
ReflectionClass->isInstantiable() is false, or when the constructor is
non-public) so skips are observable; continue to respect
container->hasDefinition / container->hasAlias as before.
- Around line 20-28: Add a brief inline comment explaining that the non-standard
parameter kernel.application_dir is a custom app-specific root directory
fallback (so reviewers understand why it’s checked) and make the path trimming
defensive for Windows by changing the rtrim call on $projectDir to strip both
forward and backslashes (e.g., rtrim($projectDir, '/\\') or otherwise normalize
using DIRECTORY_SEPARATOR) before building $endpointDirectory; keep the existing
fallback logic intact and update the comment near $projectDir,
rtrim($projectDir, ...), and $endpointDirectory to reflect the change.
- Around line 28-33: Replace the fragile hardcoded vendor path logic in
RegisterEndpointsPass that builds $endpointDirectory and uses glob on
$endpointFiles with a Composer-aware discovery: load the project's
vendor/autoload.php, call getPrefixesPsr4() on the
Composer\Autoload\ClassLoader, find the PSR-4 prefix used by the REST client
(e.g. the Endpoint namespace used by the client), iterate each mapped directory
root returned for that prefix, skip non-directories, and run the existing
file-globbing/registration logic inside that loop; this removes assumptions
about vendor-dir, package name, and internal layout and reuses the autoloader to
locate the true package paths.
In `@src/DependencyInjection/PhpListFrontendExtension.php`:
- Around line 58-82: Remove the large commented-out security configuration block
that begins with $container->prependExtensionConfig('security', ...) inside the
PhpListFrontendExtension class; delete the entire commented segment so only the
active configuration remains, keeping the file focused and preventing drift from
config/packages/security.yaml.
In `@tests/Unit/Service/SubscriberCollectionNormalizerTest.php`:
- Around line 91-108: The test testNormalizeNullItems in
SubscriberCollectionNormalizerTest is misleading because
SubscriberCollection::$items is typed as array and the test only covers an empty
items case; either remove this test or rename it to reflect the actual behavior
(e.g., testNormalizeEmptyItems) and update its comment and assertions
accordingly so it no longer claims to exercise null items; locate the test
method testNormalizeNullItems and either delete it or rename it and adjust the
inline comment that mentions null to accurately describe the empty-array
scenario, leaving the call to $this->normalizer->normalize($collection, null,
null) and the assertions intact.
🪄 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: 0bf3e427-fb67-4a1b-8fbb-fb12b4866ceb
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (47)
.github/workflows/ci.ymlREADME.mdassets/router/index.jsassets/vue/api.jsassets/vue/components/base/BaseBadge.spec.jsassets/vue/components/base/BaseButton.spec.jsassets/vue/components/base/BaseIcon.spec.jsassets/vue/components/base/BaseProgressBar.spec.jsassets/vue/components/base/CkEditorField.vueassets/vue/components/campaigns/CampaignDirectory.vueassets/vue/components/campaigns/ViewCampaignModal.vueassets/vue/components/lists/ListDirectory.vueassets/vue/components/sidebar/AppSidebar.vueassets/vue/components/subscribers/SubscriberDirectory.vueassets/vue/layouts/AdminLayout.vueassets/vue/views/CampaignEditView.vueassets/vue/views/CampaignsView.vuecomposer.jsonconfig/packages/security.yamlconfig/services.ymlopenapi.jsonpackage.jsonphpunit.xml.distsrc/Controller/AuthController.phpsrc/Controller/CampaignsController.phpsrc/Controller/DashboardController.phpsrc/Controller/ListsController.phpsrc/Controller/SubscribersController.phpsrc/DependencyInjection/Compiler/RegisterEndpointsPass.phpsrc/DependencyInjection/PhpListFrontendExtension.phpsrc/EventSubscriber/AuthGateSubscriber.phpsrc/PhpListFrontendBundle.phpsrc/Security/SessionAuthenticator.phptemplates/auth/login.html.twigtemplates/base.html.twigtemplates/spa.html.twigtests/Integration/Controller/AuthControllerTest.phptests/Integration/Controller/CampaignsControllerTest.phptests/Integration/Controller/DashboardControllerPantherTest.phptests/Integration/Controller/DashboardControllerTest.phptests/Integration/Controller/ListsControllerPantherTest.phptests/Integration/Controller/ListsControllerTest.phptests/Unit/Controller/AuthControllerTest.phptests/Unit/Security/SessionAuthenticatorTest.phptests/Unit/Service/SubscriberCollectionNormalizerTest.phptests/Unit/Service/SubscriberExportRequestFactoryTest.phpvitest.config.mjs
✅ Files skipped from review due to trivial changes (3)
- phpunit.xml.dist
- README.md
- templates/spa.html.twig
🚧 Files skipped from review as they are similar to previous changes (10)
- assets/vue/components/sidebar/AppSidebar.vue
- assets/vue/layouts/AdminLayout.vue
- package.json
- composer.json
- src/EventSubscriber/AuthGateSubscriber.php
- config/services.yml
- tests/Unit/Controller/AuthControllerTest.php
- src/Controller/AuthController.php
- templates/auth/login.html.twig
- assets/vue/components/subscribers/SubscriberDirectory.vue
| echo "PANTHER_CHROME_BINARY=/usr/bin/google-chrome" >> .env.test | ||
| - name: Start mysql service | ||
| run: sudo /etc/init.d/mysql start | ||
| sudo apt install socat |
There was a problem hiding this comment.
Make the socat install non-interactive.
sudo apt install socat can prompt for confirmation in CI. Use apt-get install -y to keep the workflow deterministic.
🔧 Proposed fix
- sudo apt install socat
+ sudo apt-get install -y socat📝 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
| sudo apt install socat | |
| sudo apt-get install -y socat |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml at line 61, Replace the interactive apt install
invocation in the CI workflow: change the line that runs "sudo apt install
socat" to a non-interactive installation using apt-get with the -y flag and
include an apt-get update beforehand (e.g., use apt-get update && apt-get
install -y socat) so the CI won't prompt for confirmation.
Comment on lines
+17
to
+27
| { path: '/campaigns/:campaignId/edit', name: 'campaign-edit', component: CampaignEditView, meta: { title: 'Edit Campaign' } }, | ||
| { path: '/lists/:listId/subscribers', name: 'list-subscribers', component: ListSubscribersView, meta: { title: 'List Subscribers' } }, | ||
| { path: '/:pathMatch(.*)*', redirect: '/' }, | ||
| ], | ||
| }); | ||
|
|
||
| router.afterEach((to) => { | ||
| const defaultTitle = 'phpList'; | ||
| const pageTitle = to.meta.title; | ||
| document.title = pageTitle ? `${defaultTitle} - ${pageTitle}` : defaultTitle; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm campaign edit title mismatch between backend title and router meta title.
rg -nP "Edit Campaign|to\.meta\.title|document\.title|campaignId" -C3 assets src testsRepository: phpList/web-frontend
Length of output: 23299
Make the campaign ID visible in the edit page title.
The backend renders Edit Campaign #123`` for /campaigns/123/edit, but the router's static `meta.title` overwrites it with just `Edit Campaign` after navigation, losing the ID. The Vue component already computes the correct title internally, so the router hook needs to support dynamic meta titles.
The proposed fix—making meta.title a function that accepts the route and checking for it in the router.afterEach hook—will solve this.
Proposed fix
- { path: '/campaigns/:campaignId/edit', name: 'campaign-edit', component: CampaignEditView, meta: { title: 'Edit Campaign' } },
+ {
+ path: '/campaigns/:campaignId/edit',
+ name: 'campaign-edit',
+ component: CampaignEditView,
+ meta: { title: (route) => `Edit Campaign #${route.params.campaignId}` },
+ },
@@
router.afterEach((to) => {
const defaultTitle = 'phpList';
- const pageTitle = to.meta.title;
+ const pageTitle = typeof to.meta.title === 'function' ? to.meta.title(to) : to.meta.title;
document.title = pageTitle ? `${defaultTitle} - ${pageTitle}` : defaultTitle;
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/router/index.js` around lines 17 - 27, The router currently overwrites
dynamic titles because meta.title is a static string for the route
'/campaigns/:campaignId/edit' (name: 'campaign-edit') and router.afterEach
always uses to.meta.title; change meta.title to accept either a string or a
function and update the router.afterEach hook (router.afterEach) to detect if
to.meta.title is a function and, if so, call it with the current route (to) to
get the pageTitle (falling back to the string value or defaultTitle) so
CampaignEditView can surface the computed "Edit Campaign `#123`" title without
losing support for static titles.
Comment on lines
+40
to
+53
| for (let pageIndex = 0; pageIndex < maxPages; pageIndex += 1) { | ||
| const response = await listClient.getLists(afterId, limit); | ||
| const items = Array.isArray(response?.items) ? response.items : []; | ||
| lists.push(...items); | ||
|
|
||
| const hasMore = response?.pagination?.hasMore === true; | ||
| const nextCursor = response?.pagination?.nextCursor; | ||
|
|
||
| if (!hasMore || !Number.isFinite(nextCursor) || nextCursor === afterId) { | ||
| break; | ||
| } | ||
|
|
||
| afterId = nextCursor; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the installed client's getLists signature and pagination shape.
fd -t f 'package.json' node_modules/@tatevikgr/rest-api-client 2>/dev/null | head -5
fd -e d.ts . node_modules/@tatevikgr/rest-api-client 2>/dev/null | xargs -I{} sh -c 'echo "== {} =="; cat "{}"' 2>/dev/null | rg -n -C3 'getLists|nextCursor|pagination'
# Fallback: search repo for any existing usage that hints at cursor type
rg -nP -C2 'nextCursor|getLists\s*\(' --type=js --type=ts --type=vueRepository: phpList/web-frontend
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for nextCursor and getLists usage without problematic rg flags
rg -n -C3 'nextCursor|getLists' assets/ src/ --max-count=20
# Check if the client package is installed and look at its metadata
if [ -d "node_modules/@tatevikgr/rest-api-client" ]; then
echo "=== Client package found ==="
ls -la node_modules/@tatevikgr/rest-api-client/ 2>/dev/null | head -20
find node_modules/@tatevikgr/rest-api-client -name "*.d.ts" -o -name "index.js" | head -10
fi
# Look at the api.js file more broadly to understand initialization
echo "=== Full fetchAllLists context ==="
cat -n assets/vue/api.js | head -60Repository: phpList/web-frontend
Length of output: 12780
Number.isFinite(nextCursor) breaks pagination with non-numeric cursors.
The API client returns nextCursor as a potentially opaque value (string, UUID, or any truthy type), not a number. Using Number.isFinite() causes this check to fail for string cursors and silently stops pagination even when hasMore is true — you'll get only the first page of results.
The rest of the codebase already uses the correct pattern (e.g., ListSubscribersView.vue:376, CampaignDirectory.vue:779). Relax the check to match:
♻️ Fix
- if (!hasMore || !Number.isFinite(nextCursor) || nextCursor === afterId) {
+ if (!hasMore || nextCursor == null || nextCursor === afterId) {
break;
}Also consider logging when the loop exits due to maxPages — silent truncation at 10k results could cause confusion later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/api.js` around lines 40 - 53, The loop stops pagination
incorrectly because Number.isFinite(nextCursor) assumes numeric cursors; replace
that condition with a null/undefined check to allow opaque (string/UUID)
cursors: change the break condition to use nextCursor == null (or nextCursor ===
undefined || nextCursor === null) and keep the existing nextCursor === afterId
check; reference the variables and call: listClient.getLists, nextCursor,
hasMore, afterId. Also add a debug/info log when the loop exits because it
reached the maxPages limit to avoid silent truncation.
Comment on lines
+28
to
+30
| const classes = wrapper.get('span').classes() | ||
| expect(classes).toContain('bg-indigo-50') | ||
| expect(classes).toContain('text-ext-wf3') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify whether `text-ext-wf3` is backed by Tailwind config and find all usages.
# Expected: Tailwind config defines an `ext-wf3` color token, or usages should be changed to a configured token.
set -euo pipefail
echo "== Tailwind config files =="
fd -i '^tailwind\.config\.(js|cjs|mjs|ts)$' -x sh -c '
printf "\n--- %s ---\n" "$1"
sed -n "1,240p" "$1"
' sh {}
echo
echo "== text-ext-wf3 / ext-wf3 usages =="
rg -n -C2 '\btext-ext-wf3\b|\bext-wf3\b|\bext_wf3\b' --glob '!node_modules/**' --glob '!vendor/**'Repository: phpList/web-frontend
Length of output: 9653
Add ext-wf3 to the Tailwind config safelist or extend colors.
The class text-ext-wf3 is used in BaseBadge.vue and other components, but it's not defined in tailwind.config.js—only ext-wf1 is. While the color exists as a CSS variable in app.css, it's missing from the safelist, which means Tailwind might purge it in production builds. Either add the color to theme.extend.colors or add text-ext-wf3 and bg-ext-wf3 to the safelist to ensure it's preserved.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/base/BaseBadge.spec.js` around lines 28 - 30, The
failing test uses the utility class text-ext-wf3 that is not preserved by
Tailwind; update tailwind.config.js so this color is never purged by either
adding the color token to theme.extend.colors (e.g., ext-wf3) or adding the
specific utilities 'text-ext-wf3' and 'bg-ext-wf3' to the safelist; modify the
config accordingly so components like BaseBadge.vue (and tests in
BaseBadge.spec.js) can use text-ext-wf3 reliably in production.
Comment on lines
+3
to
+16
| <label | ||
| v-if="label" | ||
| :for="fieldId" | ||
| class="mb-1 block text-sm font-medium text-slate-700" | ||
| > | ||
| {{ label }} | ||
| </label> | ||
|
|
||
| <ckeditor | ||
| :id="fieldId" | ||
| v-model="localValue" | ||
| :editor="ClassicEditor" | ||
| :config="editorConfig" | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "CkEditorField.vue"Repository: phpList/web-frontend
Length of output: 234
🏁 Script executed:
cat -n assets/vue/components/base/CkEditorField.vueRepository: phpList/web-frontend
Length of output: 3244
🌐 Web query:
CKEditor5 Vue config.root.label accessibility
💡 Result:
In CKEditor 5 Vue integration, the config.root.label option sets an accessible label for the editor's editing root (the contenteditable div). This label is used as the aria-label attribute on the editable area, improving screen reader accessibility by describing the editing area (e.g., distinguishing multiple editors on a page). If unspecified, a default like "Rich Text Editor. Editing area [name]" is used. Use it in the Vue component's config: const config = { root: { label: 'Article main content' }, // other config options... }; This applies to all standard CKEditor 5 editor types (ClassicEditor, InlineEditor, etc.) used with the Vue component, as the config is passed directly to Editor.create. CKEditor 5 meets WCAG 2.2 AA standards, and this helps with ARIA requirements for multiple editable regions.
Citations:
- 1: https://docs.ckeditor.com/ckeditor5/latest/api/module_core_editor_editorconfig-RootConfig.html
- 2: https://ckeditor.com/docs/ckeditor5/latest/api/module_core_editor_editorconfig-RootConfig.html
- 3: https://ckeditor.com/docs/ckeditor5/latest/installation/integrations/vuejs-v3.html
- 4: https://github.com/ckeditor/ckeditor5/blob/master/docs/getting-started/integrations/vuejs-v3.md
Pass the visible label into CKEditor's editable area.
The <label for> won't label CKEditor's focusable editing root—CKEditor exposes this through config.root.label. Without it, screen readers get a generic default instead of your actual label text. CKEditor 5 specifically documents root.label as the aria-label for the editing area and it's required for WCAG 2.2 AA compliance.
Proposed fix
-const editorConfig = {
+const editorConfig = computed(() => ({
licenseKey: 'GPL',
+ root: {
+ label: props.label || 'Rich text editor'
+ },
plugins: [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/base/CkEditorField.vue` around lines 3 - 16, The
CKEditor editable root isn't receiving the visible label, so screen readers see
a generic name; update the component to inject the visible label into the editor
configuration by ensuring editorConfig.root.label is set to the component's
label (e.g., inside CkEditorField.vue merge or assign { root: { label: label } }
into the existing editorConfig before passing it to the <ckeditor> component),
referencing fieldId and ClassicEditor as before so the editable area receives
the aria-label matching the visible label.
Comment on lines
+35
to
+36
| #[Route('/{campaignId}/edit', name: 'edit', methods: ['GET'])] | ||
| public function edit(Request $request, int $campaignId): Response |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/Controller/CampaignsController.php | head -50Repository: phpList/web-frontend
Length of output: 1928
🏁 Script executed:
rg '#\[Route.*{.*}' -A 1 src/Controller/ --type phpRepository: phpList/web-frontend
Length of output: 171
🏁 Script executed:
rg 'Route.*\{' src/Controller/ -A 1 --type phpRepository: phpList/web-frontend
Length of output: 512
🏁 Script executed:
rg -A 3 'Route.*listId' src/Controller/ListsController.php --type phpRepository: phpList/web-frontend
Length of output: 295
Constrain campaignId to numeric values.
Without a route requirement, /campaigns/not-a-number/edit matches the route and fails during type coercion instead of returning a clean 404. The same issue exists in ListsController for {listId}.
🔧 Proposed fix
- #[Route('/{campaignId}/edit', name: 'edit', methods: ['GET'])]
+ #[Route('/{campaignId}/edit', name: 'edit', requirements: ['campaignId' => '\d+'], methods: ['GET'])]📝 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
| #[Route('/{campaignId}/edit', name: 'edit', methods: ['GET'])] | |
| public function edit(Request $request, int $campaignId): Response | |
| #[Route('/{campaignId}/edit', name: 'edit', requirements: ['campaignId' => '\d+'], methods: ['GET'])] | |
| public function edit(Request $request, int $campaignId): Response |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Controller/CampaignsController.php` around lines 35 - 36, The route for
the edit action currently allows non-numeric segments so requests like
/campaigns/not-a-number/edit reach CampaignsController::edit(Request $request,
int $campaignId): Response and fail during parameter coercion; add a route
requirement to constrain {campaignId} to digits (e.g. requirement for "\d+") on
the #[Route(...)] attribute for the edit method to return a 404 for non-numeric
IDs; apply the same change to ListsController where the route uses {listId} to
constrain that parameter to numeric values as well.
Comment on lines
+38
to
+39
| #[Route('/{listId}/subscribers', name: 'list_subscribers', methods: ['GET'])] | ||
| public function view(Request $request, int $listId): JsonResponse|Response |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/Controller/ListsController.php | sed -n '35,45p'Repository: phpList/web-frontend
Length of output: 612
🏁 Script executed:
rg "Route\(" src/Controller/ListsController.php -A 1Repository: phpList/web-frontend
Length of output: 430
🏁 Script executed:
rg "requirements:" src/Controller/ -c | head -20Repository: phpList/web-frontend
Length of output: 46
Constrain listId to numeric values.
The route lacks a requirement constraint, so /lists/not-a-number/subscribers will match and fail at the controller level instead of returning a 404. Add a numeric constraint to the route pattern.
🔧 Proposed fix
- #[Route('/{listId}/subscribers', name: 'list_subscribers', methods: ['GET'])]
+ #[Route('/{listId}/subscribers', name: 'list_subscribers', requirements: ['listId' => '\d+'], methods: ['GET'])]📝 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
| #[Route('/{listId}/subscribers', name: 'list_subscribers', methods: ['GET'])] | |
| public function view(Request $request, int $listId): JsonResponse|Response | |
| #[Route('/{listId}/subscribers', name: 'list_subscribers', requirements: ['listId' => '\d+'], methods: ['GET'])] | |
| public function view(Request $request, int $listId): JsonResponse|Response |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Controller/ListsController.php` around lines 38 - 39, The route attribute
on the view method is missing a requirement for the listId placeholder so
non-numeric paths reach the controller; update the #[Route(...)] attribute on
the view(Request $request, int $listId) method in ListsController to include a
requirements entry that constrains listId to digits only (e.g. using a \d+ or
[0-9]+ pattern) so invalid slugs return a 404 before the controller is invoked.
| $client->getCookieJar()->clear(); | ||
| $client->request('GET', '/login'); | ||
|
|
||
| $client->takeScreenshot('var/screenshots/login-dashboard.png'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether screenshot output paths are committed or created by test setup.
# Expectation if the screenshot call stays: var/screenshots exists or is created by bootstrap/config.
test -d var/screenshots && printf 'var/screenshots exists\n' || printf 'var/screenshots is missing\n'
rg -n -C2 'var/screenshots|takeScreenshot|mkdir\s*\([^)]*screenshots' .Repository: phpList/web-frontend
Length of output: 1086
🏁 Script executed:
#!/bin/bash
# Check for phpunit configuration and test bootstrap files
echo "=== phpunit.xml/config ==="
find . -maxdepth 2 -name "phpunit*.xml" -o -name "phpunit*.dist"
echo -e "\n=== Bootstrap files ==="
find . -path ./node_modules -prune -o -name "*bootstrap*" -type f | head -20
echo -e "\n=== CI configurations ==="
find . -maxdepth 3 \( -name "*.yml" -o -name "*.yaml" \) | grep -E "(github|gitlab|circleci|travis)" | head -10
echo -e "\n=== Var directory structure ==="
find . -maxdepth 2 -type d -name "var"
echo -e "\n=== Test setup methods in test files ==="
rg -n "setUp|setUpBeforeClass|mkdir|createDir" tests/ -A 2 | head -40Repository: phpList/web-frontend
Length of output: 2170
🏁 Script executed:
#!/bin/bash
# Check phpunit configuration
echo "=== phpunit.xml.dist content ==="
cat phpunit.xml.dist
echo -e "\n=== CI workflow for test execution ==="
cat .github/workflows/ci.yml | head -80
echo -e "\n=== AuthControllerTest.php full file ==="
cat tests/Integration/Controller/AuthControllerTest.php
echo -e "\n=== DashboardControllerPantherTest.php (similar test) ==="
cat tests/Integration/Controller/DashboardControllerPantherTest.phpRepository: phpList/web-frontend
Length of output: 6495
Remove the unconditional screenshot write to avoid CI failures.
The var/screenshots directory doesn't exist and isn't created by the test bootstrap, phpunit config, or CI workflow. When this test runs, the takeScreenshot() call will fail unless the directory is manually created beforehand. Either remove it or conditionally create the directory only when screenshots are explicitly enabled (e.g., via an environment variable).
Suggested fix
- $client->takeScreenshot('var/screenshots/login-dashboard.png');
-
$this->assertPageTitleContains('phpList - Login');Note: DashboardControllerPantherTest.php has the same pattern at line 31 and should get the same treatment.
📝 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
| $client->takeScreenshot('var/screenshots/login-dashboard.png'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Integration/Controller/AuthControllerTest.php` at line 25, Remove the
unconditional screenshot write in AuthControllerTest (the
$client->takeScreenshot call) and instead either delete the call or guard it
behind a screenshot-enabled check (e.g., read an environment flag like
SCREENSHOTS_ENABLED) so the test only attempts $client->takeScreenshot when
screenshots are explicitly enabled; apply the same change to
DashboardControllerPantherTest where the same pattern appears (the
$client->takeScreenshot invocation) and if you keep the feature, ensure the
directory is created only when the flag is set.
| ]); | ||
| $client->getCookieJar()->clear(); | ||
| $client->request('GET', '/login'); | ||
| $client->takeScreenshot('var/screenshots/login-page.png'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether screenshot output directories are present or only referenced by tests.
find var -maxdepth 3 -type d 2>/dev/null | sort
rg -nP "takeScreenshot\(|var/screenshots" -C2Repository: phpList/web-frontend
Length of output: 1039
🏁 Script executed:
# Check if var directory exists and what's in it
ls -la var/ 2>&1 || echo "var/ directory does not exist"
# Check for .gitkeep or placeholder files
find . -name ".gitkeep" -o -name "*.gitkeep" 2>/dev/null | head -20
# Check if var/ is in .gitignore
cat .gitignore 2>/dev/null | grep -E "^var|/var" || echo "No var/* pattern in .gitignore"Repository: phpList/web-frontend
Length of output: 151
🏁 Script executed:
# Check for any CI configuration that might create directories
find . -maxdepth 2 -type f \( -name "*.yml" -o -name "*.yaml" -o -name "*.json" -o -name "Makefile" -o -name "docker-compose*" \) 2>/dev/null | head -20Repository: phpList/web-frontend
Length of output: 163
🏁 Script executed:
# Look for any test setup/bootstrap files that might create the directory
find tests -maxdepth 2 -type f \( -name "bootstrap.php" -o -name "setup.php" -o -name "*Fixture*" \) 2>/dev/nullRepository: phpList/web-frontend
Length of output: 46
🏁 Script executed:
# Check .gitignore for var/ pattern
cat .gitignoreRepository: phpList/web-frontend
Length of output: 304
🏁 Script executed:
# Look for GitHub Actions or CI configurations
find .github -type f 2>/dev/null || echo "No .github directory"
find . -maxdepth 1 -name "*.yml" -o -name "*.yaml" 2>/dev/nullRepository: phpList/web-frontend
Length of output: 190
🏁 Script executed:
# Search for any Panther or test documentation
find . -maxdepth 2 -type f \( -name "README*" -o -name "TESTING*" \) 2>/dev/nullRepository: phpList/web-frontend
Length of output: 75
🏁 Script executed:
# Check for any directory creation in test files
rg "mkdir|createDir|->getDirectory" tests/ -i -A 1 -B 1Repository: phpList/web-frontend
Length of output: 46
🏁 Script executed:
# Check the GitHub Actions CI workflow
cat .github/workflows/ci.ymlRepository: phpList/web-frontend
Length of output: 6088
🏁 Script executed:
# Look for Panther-specific configuration or test setup
rg "Panther|Browser|WebDriver" tests/ -B 2 -A 2Repository: phpList/web-frontend
Length of output: 4320
🏁 Script executed:
# Check phpunit configuration or test setup
cat phpunit.xml* 2>/dev/null || cat tests/phpunit.xml 2>/dev/null || echo "No phpunit.xml found in expected locations"Repository: phpList/web-frontend
Length of output: 653
🏁 Script executed:
# Look for any Symfony console commands or test fixtures that might handle setup
rg "public function setUp|testClassSetUp" tests/ -A 3Repository: phpList/web-frontend
Length of output: 46
🏁 Script executed:
# Check if Panther has any documentation or mentions about directory creation
rg "screenshot|directory|mkdir|createDir" composer.lock vendor/ 2>/dev/null | head -30Repository: phpList/web-frontend
Length of output: 46
🏁 Script executed:
# Look at the actual test file to see the full context
cat -n tests/Integration/Controller/DashboardControllerPantherTest.phpRepository: phpList/web-frontend
Length of output: 1640
Create the screenshot directory before writing to it.
takeScreenshot('var/screenshots/login-page.png') will fail if the var/screenshots directory doesn't exist. Since var/ is gitignored and never created during test setup, this test can fail with a directory-not-found error on any fresh checkout before reaching the actual assertions.
Suggested fix
+ $screenshotPath = 'var/screenshots/login-page.png';
+ if (! is_dir(dirname($screenshotPath))) {
+ mkdir(dirname($screenshotPath), 0775, true);
+ }
- $client->takeScreenshot('var/screenshots/login-page.png');
+ $client->takeScreenshot($screenshotPath);📝 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
| $client->takeScreenshot('var/screenshots/login-page.png'); | |
| $screenshotPath = 'var/screenshots/login-page.png'; | |
| if (! is_dir(dirname($screenshotPath))) { | |
| mkdir(dirname($screenshotPath), 0775, true); | |
| } | |
| $client->takeScreenshot($screenshotPath); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Integration/Controller/DashboardControllerPantherTest.php` at line 31,
The test calls $client->takeScreenshot('var/screenshots/login-page.png') but
never ensures the var/screenshots directory exists; update the
DashboardControllerPantherTest (where takeScreenshot is invoked) to create the
directory before calling takeScreenshot (e.g., using mkdir or Symfony
Filesystem) so that var/screenshots is created with correct permissions when
missing; ensure this creation happens just prior to the takeScreenshot call in
the test setup or the test method so the screenshot write cannot fail.
Comment on lines
+26
to
+30
| $client = static::createPantherClient([ | ||
| 'browser' => static::CHROME, | ||
| 'connection_timeout_in_ms' => 10000, | ||
| ]); | ||
| $client->request('GET', $path); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find Panther tests that may authenticate or rely on cookies.
# Expectation: anonymous-redirect tests should clear cookies consistently.
rg -n -C3 'createPantherClient|getCookieJar\(\)->clear|auth_token|/login|submit|request\(.+POST' testsRepository: phpList/web-frontend
Length of output: 20380
🏁 Script executed:
cat -n tests/Integration/Controller/ListsControllerPantherTest.phpRepository: phpList/web-frontend
Length of output: 1707
Clear browser cookies to ensure anonymous state before testing redirect.
This test verifies anonymous redirect behavior, but doesn't clear cookies before the request. If Panther reuses the browser process or cookies persist from previous test runs, a residual auth_token could prevent the redirect, causing the test to fail or become flaky. Other Panther tests in the suite (AuthControllerTest, DashboardControllerPantherTest) consistently call getCookieJar()->clear() before testing protected routes—add it here too for consistency.
Suggested fix
$client = static::createPantherClient([
'browser' => static::CHROME,
'connection_timeout_in_ms' => 10000,
]);
+ $client->getCookieJar()->clear();
$client->request('GET', $path);📝 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
| $client = static::createPantherClient([ | |
| 'browser' => static::CHROME, | |
| 'connection_timeout_in_ms' => 10000, | |
| ]); | |
| $client->request('GET', $path); | |
| $client = static::createPantherClient([ | |
| 'browser' => static::CHROME, | |
| 'connection_timeout_in_ms' => 10000, | |
| ]); | |
| $client->getCookieJar()->clear(); | |
| $client->request('GET', $path); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Integration/Controller/ListsControllerPantherTest.php` around lines 26
- 30, The test may inherit cookies from a reused Panther browser causing an
existing auth_token to bypass the anonymous redirect; before calling
$client->request('GET', $path) clear the browser cookie jar by invoking
$client->getCookieJar()->clear() (i.e., call getCookieJar()->clear() on the
$client returned by createPantherClient(...) immediately after creation) so the
request runs in a clean anonymous state.
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
Bug Fixes
Style
Chores
Thanks for contributing to phpList!