Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Vue 3 SPA frontend (router, components, layouts, Tailwind/PostCSS, webpack and package.json updates) plus a JS REST API client. Server-side adds Symfony controllers (Dashboard, Subscribers), security config, ApiSessionListener, Unauthorized handling changes, service registrations, AuthController.about endpoint, and event subscribers. New templates for SPA and login, Apache vhost, and many Vue components (sidebar, dashboard, subscribers, charts, base components). Tests adjusted for UI and auth changes. Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant Symfony
participant Session
participant RESTClient as Browser REST Client
participant Backend as REST API
User->>Browser: Request /
Browser->>Symfony: GET /
Symfony->>Session: read auth_token
alt session present
Symfony->>Browser: render spa.html.twig (api_token, api_base_url)
Browser->>Browser: mount Vue app, init router
Browser->>RESTClient: configure client (baseUrl + token)
Browser->>Backend: GET /subscribers?filters
Backend-->>Browser: subscribers JSON
Browser->>Browser: render SubscriberDirectory -> SubscriberTable
else no session
Symfony->>Browser: redirect /login
end
sequenceDiagram
actor User
participant Browser
participant AuthController
participant AuthClient
participant Session
participant Symfony
User->>Browser: Submit login form
Browser->>AuthController: POST /login (credentials)
AuthController->>AuthClient: login(username,password)
AuthClient-->>AuthController: { token, id, expiry_date }
AuthController->>Session: set auth_token, auth_id, auth_expiry_date
AuthController->>Browser: Redirect to /
Browser->>Symfony: GET /
Symfony->>Browser: render spa.html.twig
sequenceDiagram
actor User
participant SubscriberDir as SubscriberDirectory (Vue)
participant FileInput
participant BrowserREST as REST API Client
participant Backend as REST API
User->>SubscriberDir: Click "Import CSV"
SubscriberDir->>FileInput: open picker
User->>FileInput: select file
FileInput->>SubscriberDir: file selected
SubscriberDir->>BrowserREST: POST /subscribers/import (file + token)
BrowserREST->>Backend: upload file
Backend-->>BrowserREST: { imported, skipped, errors }
BrowserREST-->>SubscriberDir: import result
SubscriberDir->>SubscriberDir: show ImportResult modal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Controller/AuthController.php (1)
53-57:⚠️ Potential issue | 🟡 MinorUnreachable
GuzzleExceptioncatch block.
GuzzleExceptionextendsException, so the first catch block will always handle Guzzle exceptions. Swap the order or consolidate into a single catch.🐛 Proposed fix
- } catch (Exception $e) { - $error = 'Invalid credentials or server error: ' . $e->getMessage(); - } catch (GuzzleException $e) { + } catch (GuzzleException $e) { + $error = 'Invalid credentials or server error: ' . $e->getMessage(); + } catch (Exception $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, The two catch blocks in AuthController (the catch for Exception $e followed by catch for GuzzleException $e) make the GuzzleException catch unreachable; either swap the order so catch (GuzzleException $e) appears before catch (Exception $e) or consolidate them into a single catch (Throwable|GuzzleException $e) / catch (Exception $e) that handles both and preserves the existing error assignment logic (the variables and message concatenation in the current catch bodies should be kept). Ensure you update the catch declarations in the method containing those try/catch blocks so Guzzle exceptions are handled as intended.
🟠 Major comments (17)
assets/vue/api.js-7-12 (1)
7-12:⚠️ Potential issue | 🟠 MajorFail fast when API base URL is missing.
Lines 7-12 log the error but still instantiate a client with an empty base URL. This can cause silent misrouting and hard-to-debug runtime failures.
🛠️ Proposed diff
if (!apiBaseUrl) { - console.error('API Base URL is not configured.'); + throw new Error('API Base URL is not configured.'); } -const client = new Client(apiBaseUrl || ''); +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 7 - 12, The code currently logs when apiBaseUrl is missing but still creates a Client with an empty string; change this to fail fast by throwing an Error (or otherwise aborting) instead of instantiating Client when apiBaseUrl is falsy: check the apiBaseUrl variable and if it's not set, throw a descriptive Error (e.g., "API base URL is not configured") before the Client constructor call so the client variable is never created with an invalid base URL.config/packages/framework.yaml-6-10 (1)
6-10:⚠️ Potential issue | 🟠 MajorRemove
enabled: truefrom session configuration—it is not a valid option in Symfony 6.4.Line 7 uses an unsupported configuration key that will cause container validation to fail. Sessions are enabled by default; the
enabledoption does not exist in the Framework Configuration Reference.🛠️ Suggested diff
framework: secret: '%kernel.secret%' http_method_override: false php_errors: log: true session: - enabled: true handler_id: null cookie_secure: auto cookie_samesite: lax🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/packages/framework.yaml` around lines 6 - 10, Remove the invalid "enabled: true" key from the session configuration (the block starting with "session:" that currently contains "enabled: true", "handler_id", "cookie_secure", "cookie_samesite"); sessions are enabled by default in Symfony 6.4, so delete the "enabled: true" entry and keep the existing "handler_id", "cookie_secure", and "cookie_samesite" settings intact to avoid container validation errors.tailwind.config.js-23-25 (1)
23-25:⚠️ Potential issue | 🟠 MajorTheme token mismatch: referenced custom colors are not defined.
Line 24 defines only
ext-wf1, but new components referenceext-wf2/ext-wf3classes. Those utilities won’t be generated unless these tokens are added (or component classes are aligned to existing tokens).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tailwind.config.js` around lines 23 - 25, The theme colors object only defines 'ext-wf1' but components reference 'ext-wf2' and 'ext-wf3', so add the missing tokens to the Tailwind theme (e.g., add 'ext-wf2' and 'ext-wf3' entries alongside 'ext-wf1' in the colors object) or change the components to use the existing 'ext-wf1' token; update the colors keys ('ext-wf1', 'ext-wf2', 'ext-wf3') used by your components (and any related utility classes) so Tailwind will generate the corresponding utilities.assets/vue/components/dashboard/CampaignsTable.vue-55-57 (1)
55-57:⚠️ Potential issue | 🟠 MajorGuard
statusbefore callingtoLowerCase().Line 56 can throw when
statusis null/undefined, causing the table render to fail for incomplete backend records.Suggested fix
const statusClass = (status) => { - const s = status.toLowerCase() + const s = String(status ?? '').toLowerCase()🤖 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 55 - 57, The statusClass function currently calls status.toLowerCase() which will throw if status is null/undefined; update statusClass to guard the input (e.g., check if status is truthy or coerce with a default like '') before calling toLowerCase(), and return an appropriate default CSS class for missing values; locate the statusClass arrow function and change its implementation to validate status, use a fallback string when converting to lowercase, and handle null/undefined by returning the default class.assets/vue/layouts/AdminLayout.vue-35-49 (1)
35-49:⚠️ Potential issue | 🟠 MajorUse a semantic button for the user-menu trigger.
Lines 35–49 use a clickable
<div>, which is not keyboard-accessible by default and lacks ARIA state. This should be a<button type="button">with expanded/menu semantics.Suggested fix
- <div - class="flex items-center gap-3 pl-2 cursor-pointer" - `@click`="toggleDropdown" - > + <button + type="button" + class="flex items-center gap-3 pl-2 cursor-pointer" + `@click`="toggleDropdown" + aria-haspopup="menu" + :aria-expanded="dropdownOpen ? 'true' : 'false'" + > <div class="flex flex-col items-end hidden sm:flex"> <span class="text-sm font-bold text-slate-800 leading-none"> {{ adminData.login_name || 'Admin User' }} </span> <span class="text-[10px] text-slate-500 mt-0.5"> {{ adminData.super_user ? 'Super Admin' : 'Administrator' }} </span> </div> <BaseIcon name="chevronDown" /> - </div> + </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/layouts/AdminLayout.vue` around lines 35 - 49, Replace the clickable div that wraps the user name and BaseIcon with a semantic <button type="button"> (keeping the same classes and `@click`="toggleDropdown") and add ARIA attributes: set aria-haspopup="menu" and bind aria-expanded to the component's dropdown state (e.g., :aria-expanded="isDropdownOpen" or whatever boolean the component uses for the menu). Ensure keyboard activation remains by keeping the `@click` handler and, if the component uses a different state name than isDropdownOpen, reference that exact symbol (toggleDropdown and the dropdown boolean used in this component). Preserve visual styling (classes like "flex items-center gap-3 pl-2 cursor-pointer") and ensure the hidden/sm:flex span block and BaseIcon remain unchanged inside the button.assets/vue/components/base/BaseCard.vue-32-33 (1)
32-33:⚠️ Potential issue | 🟠 Major
variantprop changes won't update classes (non-reactive derivation).Lines 32–33 derive class strings once from
props.variant. If the parent updates thevariantprop, the rendered classes stay stale. Usecomputedfor reactive derivation.Suggested fix
<script setup> +import { computed } from 'vue' const props = defineProps({-const cardClasses = cardVariantMap[props.variant] || cardVariantMap.default -const bodyClasses = bodyVariantMap[props.variant] || bodyVariantMap.default +const cardClasses = computed(() => cardVariantMap[props.variant] || cardVariantMap.default) +const bodyClasses = computed(() => bodyVariantMap[props.variant] || bodyVariantMap.default)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/base/BaseCard.vue` around lines 32 - 33, The class strings cardClasses and bodyClasses are derived once from props.variant and so won't update when the prop changes; change both to computed getters that return cardVariantMap[props.variant] || cardVariantMap.default and bodyVariantMap[props.variant] || bodyVariantMap.default respectively so they react to prop updates, and ensure computed is imported from 'vue' (template usage will auto-unwrap the refs).src/Security/SessionAuthenticator.php-39-43 (1)
39-43:⚠️ Potential issue | 🟠 MajorDo not authenticate on session-key presence alone.
Line [39] only checks key existence, then Line [41] grants an authenticated admin user. Empty or malformed token values should fail authentication.
Proposed fix
- if ($session->has('auth_token')) { + $authToken = $session->get('auth_token'); + if (is_string($authToken) && $authToken !== '') { // 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']); }); return new SelfValidatingPassport($userBadge); } - throw new AuthenticationException('No auth token in session'); + throw new AuthenticationException('Missing or invalid auth token in session');Also applies to: 48-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Security/SessionAuthenticator.php` around lines 39 - 43, The code in SessionAuthenticator that currently authenticates simply because $session->has('auth_token') must be changed to read and validate the token value (e.g., $session->get('auth_token')) and reject empty or malformed values before creating a UserBadge; specifically, in the block that constructs the UserBadge('session-user', ...) and in the similar block at lines around 48, ensure you retrieve the token string, check it is non-empty and matches expected format or validate it against your token store/session validator, and only then build a user with appropriate roles (do not automatically grant ROLE_ADMIN); if validation fails, abort authentication by throwing the appropriate AuthenticationException or returning null per SessionAuthenticator's contract.assets/vue/components/charts/LineChart.vue-29-29 (1)
29-29:⚠️ Potential issue | 🟠 MajorHarden chart path generation against malformed or mismatched series input.
Line [81] and Line [87] assume every series has a valid
dataarray aligned to labels. Malformed data can throw at runtime, and an emptycolorsarray makes Line [29] resolve to an undefined stroke.Proposed fix
<polyline v-for="(path, idx) in paths" :key="'series-' + idx" :points="path" fill="none" stroke-width="1.8" - :stroke="seriesColors[idx % seriesColors.length]" + :stroke="seriesColors.length ? seriesColors[idx % seriesColors.length] : '#2563eb'" /> @@ const paths = computed(() => { if (!props.series.length || !props.labels.length) return [] const pointCount = props.labels.length - const allValues = props.series.flatMap((s) => s.data) + const normalizedSeries = props.series.map((s) => + Array.isArray(s?.data) ? s.data.slice(0, pointCount) : [], + ) + const allValues = normalizedSeries.flat() + if (!allValues.length) return [] const max = Math.max(...allValues) const min = Math.min(...allValues) const range = max === min ? 1 : max - min - return props.series.map((s) => { - return s.data + return normalizedSeries.map((data) => { + return data .map((value, index) => { const x = pointCount === 1 ? 50 : (index / (pointCount - 1)) * 100 const normalized = (value - min) / range const y = 35 - normalized * 25 // padding top/bottom return `${x},${y}`Also applies to: 81-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/charts/LineChart.vue` at line 29, Ensure the chart code validates inputs and provides fallbacks: when computing stroke use a fallback color if seriesColors is empty (e.g., defaultStroke) so :stroke="seriesColors[idx % seriesColors.length]" never yields undefined; when building paths (the code that iterates series and indexes into series[i].data against labels at the path-generation logic around the functions handling series and labels, lines ~81–95) guard against missing or non-array series entries and mismatched lengths by skipping series with no valid Array.isArray(series[i].data) or by clamping/index-checking before accessing data[j], and handle empty labels gracefully (return empty path or placeholder). Update the code paths that reference series, series[i].data, labels and seriesColors to validate and fallback rather than assuming well-formed input.assets/styles/app.css-1-2 (1)
1-2:⚠️ Potential issue | 🟠 MajorRemove legacy
@tailwind utilities;directive — v4 requires only@import "tailwindcss";Tailwind v4 no longer uses
@tailwind base;,@tailwind components;, or@tailwind utilities;. The single@import "tailwindcss";statement is the canonical entrypoint. Line 2 should be removed.Additionally, the
.stylelintrc.jsonconfig does not includeignoreAtRulesfor Tailwind v4 directives (@theme,@source), which would cause linting warnings if stylelint is run.♻️ Proposed fix in this file
`@import` "tailwindcss"; -@tailwind utilities; `@theme` {🤖 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 legacy Tailwind v3 directive from assets/styles/app.css by deleting the line containing "@tailwind utilities;" and leave only the canonical entry "@import \"tailwindcss\";"; then update the Stylelint config (.stylelintrc.json) to add an ignoreAtRules entry that includes Tailwind v4 directives such as "@theme" and "@source" (and any other project-specific Tailwind at-rules) so Stylelint no longer warns about these directives.assets/vue/components/sidebar/SidebarNavItem.vue-34-47 (1)
34-47: 🛠️ Refactor suggestion | 🟠 MajorRemove unused imports and computed property.
The
computedimport,useRoute, and the computedisActive(line 47) are unused—the template uses the slot-providedisActivefromRouterLinkinstead.🧹 Suggested cleanup
<script setup> -import { computed } from 'vue' -import { useRoute } from 'vue-router' import BaseIcon from '../base/BaseIcon.vue' import BaseBadge from '../base/BaseBadge.vue' import { useSidebar } from '../../composables/useSidebar' const { closeSidebar } = useSidebar() const props = defineProps({ item: { type: Object, required: true }, }) - -const route = useRoute() -const isActive = computed(() => route.path === props.item.route) </script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/sidebar/SidebarNavItem.vue` around lines 34 - 47, Remove the unused reactive pieces: delete the unused imports "computed" and "useRoute" and remove the computed declaration "const isActive = computed(() => route.path === props.item.route)"; keep the existing defineProps and the "useSidebar" usage (closeSidebar) and ensure no other code references "isActive" or "route" before committing.src/EventSubscriber/UnauthorizedSubscriber.php-36-59 (1)
36-59:⚠️ Potential issue | 🟠 MajorSession invalidation logic may prevent flash message from persisting.
The session is invalidated at line 38, which clears all session data. Then at line 53-58, the code attempts to add a flash message. While
invalidate()starts a new session, the subsequent redirect will lose the flash message if the session isn't properly persisted before the response.Consider restructuring to add the flash message before invalidation, or only invalidate after handling the flash:
🔧 Suggested fix
public function onKernelException(ExceptionEvent $event): void { $exception = $event->getThrowable(); if ($exception instanceof AuthenticationException) { $request = $event->getRequest(); + $loginUrl = $this->urlGenerator->generate('login'); - if ($request->hasSession()) { - $session = $request->getSession(); - $session->invalidate(); - } - - $loginUrl = $this->urlGenerator->generate('login'); - if ($request->isXmlHttpRequest()) { + if ($request->hasSession()) { + $request->getSession()->invalidate(); + } $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.'); } + $session->invalidate(); } $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 session is invalidated before a flash is added, so the flash (set via $session->getFlashBag()->add(...)) is lost; in UnauthorizedSubscriber's handler, move the flash creation before calling $session->invalidate() or avoid full invalidation and instead regenerate the session while preserving flashes (e.g., store the flash message, call $session->invalidate(), then re-set the flash on the new session), and keep the Ajax branch returning the JsonResponse with the generated $loginUrl unchanged.assets/vue/components/subscribers/SubscriberTable.vue-50-55 (1)
50-55:⚠️ Potential issue | 🟠 MajorLabel the row action buttons.
These icon-only controls are the only way to open a subscriber, but assistive tech will only announce “button” here. Add an
aria-labelso the action is actually discoverable.♿ Suggested fix
<button + type="button" + :aria-label="`View subscriber ${subscriber.email}`" class="text-slate-400 hover:text-slate-600" `@click`="emit('view', subscriber.id)" > <BaseIcon name="eye" class="w-4 h-4" /> </button><button + type="button" + :aria-label="`View subscriber ${subscriber.email}`" class="text-slate-400 hover:text-slate-600 mr-2" `@click`="emit('view', subscriber.id)" > <BaseIcon name="eye" class="w-4 h-4" /> </button>Also applies to: 73-78
🤖 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 50 - 55, Add accessible labels to the icon-only row action buttons in SubscriberTable.vue: locate the button elements that call emit('view', subscriber.id) (and the similar button around the other occurrence) and add an appropriate aria-label (e.g., aria-label="View subscriber" or include subscriber.name/id for specificity) so assistive technologies announce the control; ensure the label is descriptive and unique if there are multiple action buttons in the row.config/services.yml-3-4 (1)
3-4:⚠️ Potential issue | 🟠 MajorDon't ship a local HTTP API fallback in shared config.
If
API_BASE_URLis missing, the app will still boot and send requests tohttp://api.phplist.local/. That makes non-local misconfigurations hard to notice and can route authenticated traffic over plain HTTP to the wrong host.🔧 Suggested fix
parameters: api_base_url: '%env(API_BASE_URL)%' - env(API_BASE_URL): 'http://api.phplist.local/'Move the local default to a local-only env file instead, so non-local environments fail fast when
API_BASE_URLis not configured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/services.yml` around lines 3 - 4, The shared config currently defines a hardcoded fallback for API_BASE_URL (api_base_url: '%env(API_BASE_URL)%' with env(API_BASE_URL): 'http://api.phplist.local/'), which masks missing configuration and routes traffic over plain HTTP; remove the env(API_BASE_URL) default from this shared services.yml so the app will fail fast when API_BASE_URL is unset, and instead place the 'http://api.phplist.local/' default only into a local-only env file (e.g., .env.local) used in development; ensure code/configurations that reference api_base_url continue to use '%env(API_BASE_URL)%' but rely on environment-specific files for defaults.src/EventListener/ApiSessionListener.php-31-45 (1)
31-45:⚠️ Potential issue | 🟠 MajorUse
hasPreviousSession()to avoid eagerly starting sessions for anonymous users.
hasSession()only confirms a session object is attached; callingget()below will start the session if it hasn't been started before, creating unnecessary sessions and cookies for anonymous traffic. This harms cacheability. Guard withhasPreviousSession()instead, which checks both that a session object exists and that a session cookie was already present in the request.Code snippet
$request = $event->getRequest(); if (!$request->hasSession()) { return; } $session = $request->getSession(); $authToken = $session->get('auth_token'); if ($authToken) { $this->apiClient->setSessionId((string) $authToken); } $authId = $session->get('auth_id'); if ($authId) { $this->apiClient->setId((int) $authId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EventListener/ApiSessionListener.php` around lines 31 - 45, Replace the eager session check using $request->hasSession() with $request->hasPreviousSession() in ApiSessionListener so you only access $request->getSession() when a prior session cookie exists; specifically, in the method that calls $request = $event->getRequest(), change the guard to use hasPreviousSession() before calling $request->getSession() and then proceed to read 'auth_token' and 'auth_id' and call $this->apiClient->setSessionId(...) and $this->apiClient->setId(...).assets/vue/components/subscribers/SubscriberDirectory.vue-180-225 (1)
180-225:⚠️ Potential issue | 🟠 MajorIgnore stale
fetchSubscribers()responses.Search, filter changes, and pagination can all overlap requests here. Without cancellation or a request token, an older response can land last and overwrite the newer result set the user is looking at.
⏱️ Suggested fix
let searchTimeout = null +let activeRequest = 0const fetchSubscribers = async (afterId = null) => { + const requestId = ++activeRequest const url = new URL('/subscribers', window.location.origin) if (afterId !== null) { url.searchParams.set('after_id', afterId) } @@ } const data = await response.json() + if (requestId !== activeRequest) { + return + } subscribers.value = data.items pagination.value = data.pagination🤖 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 180 - 225, The fetchSubscribers flow can return stale responses; add an AbortController to cancel any in-flight request before starting a new one: create a module-scoped variable (e.g., currentFetchController), and in fetchSubscribers() abort and replace it with a new AbortController, pass its signal to fetch(..., { signal }), and in the catch block ignore DOMException/AbortError so aborted requests don't log errors or overwrite state; ensure callers like handleSearch (and any filter/pagination handlers that call fetchSubscribers) continue to call fetchSubscribers() so the abort logic centrally prevents stale responses.src/Controller/SubscribersController.php-104-104 (1)
104-104:⚠️ Potential issue | 🟠 MajorMissing default for
limitin export may cause issues.
getInt('limit')returns0if the parameter is absent. Depending on the API behavior, this could either fetch all records (potential performance/DoS issue) or none (unexpected empty export).🛡️ Proposed fix - add sensible default or max limit
- $collection = $this->subscribersClient->getSubscribers($filter, 0, $request->query->getInt('limit')); + $limit = $request->query->getInt('limit', 10000); // sensible max for export + $collection = $this->subscribersClient->getSubscribers($filter, 0, $limit);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/SubscribersController.php` at line 104, The call to $this->subscribersClient->getSubscribers($filter, 0, $request->query->getInt('limit')) uses request->query->getInt('limit') which returns 0 when absent and can cause full or empty exports; change this to supply a sensible default and enforce a maximum: read the limit with a fallback (e.g. $limit = $request->query->getInt('limit', <default>)) then clamp it (e.g. min($limit, <max>)) before passing to getSubscribers, updating the invocation in SubscribersController::getSubscribers call site to use the validated $limit value.src/Controller/AuthController.php-75-79 (1)
75-79:⚠️ Potential issue | 🟠 MajorMissing authentication check on
/admin-aboutendpoint.The
about()method exposes session user data without verifying the user is authenticated. If called without a valid session,getSessionUser()may throw or return unexpected data. Consider adding a session check or applying a security attribute.🛡️ Proposed fix
#[Route('/admin-about', name: 'admin_about')] -public function about(): JsonResponse +public function about(Request $request): JsonResponse { + if (!$request->getSession()->has('auth_token')) { + return new JsonResponse(['error' => 'Unauthorized'], Response::HTTP_UNAUTHORIZED); + } return new JsonResponse($this->authClient->getSessionUser()->toArray()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/AuthController.php` around lines 75 - 79, The about() action on the /admin-about route returns session data without verifying authentication; update the method to check the current session/user before calling $this->authClient->getSessionUser(), returning a 401/403 JsonResponse when not authenticated (or apply a security attribute like #[IsGranted('ROLE_ADMIN')] or #[Security(...)] to the controller/method) and only call getSessionUser()->toArray() when the user is present; reference the about() method, $this->authClient and getSessionUser() when making the change.
🟡 Minor comments (6)
assets/vue/components/base/BaseProgressBar.vue-27-29 (1)
27-29:⚠️ Potential issue | 🟡 Minor
wrapperStyleis not reactive and won't update ifheightprop changes.The object is created once at setup time, capturing
props.heightat that moment. If a parent component dynamically changes theheightprop, the wrapper height won't update.🔧 Proposed fix using a computed property
<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>Alternatively, inline the style directly in the template:
:style="{ height }".🤖 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, The wrapperStyle object is created once in setup and freezes props.height, so update it to be reactive by making wrapperStyle a computed that returns { height: props.height } (or apply the style inline in the template as :style="{ height }"); locate the wrapperStyle declaration in BaseProgressBar.vue (inside setup) and replace the static object with a computed(() => ({ height: props.height })) so changes to the height prop update the DOM.assets/vue/components/subscribers/SubscriberFilters.vue-3-13 (1)
3-13:⚠️ Potential issue | 🟡 MinorSet an explicit button type to prevent accidental form submits.
Line 3 defines a
<button>withouttype. In form contexts this defaults tosubmit, so clicking a filter can trigger unintended submits.Suggested fix
- <button + <button + type="button" v-for="filter in filters"🤖 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 3 - 13, The button rendered in the v-for (iterating over filters) is missing an explicit type, causing it to default to "submit" in form contexts; update the <button> inside the v-for that uses :key="filter.id" and `@click`="toggleFilter(filter.id)" to include type="button" so clicks on the filter buttons won't trigger unintended form submissions (leave existing classes, bindings, and the toggleFilter handler unchanged).templates/auth/login.html.twig-38-38 (1)
38-38:⚠️ Potential issue | 🟡 MinorReplace the placeholder links before shipping.
These
href="#"anchors advertise real actions but only jump the page to the top. Either wire them to actual routes or render them as plain text until the destinations exist.Also applies to: 57-59
🤖 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 in the login template uses a placeholder href="#" which should be replaced before shipping; update the anchor(s) (e.g., the "Forgot password?" link and the other anchors at lines 57-59 in templates/auth/login.html.twig) to either point to the actual route helper or URL for the password-reset / target pages (use your framework's path/url function) or convert them to non-clickable text (e.g., a <span> with the same classes) until the destination exists so the UI doesn't mislead users.assets/vue/components/subscribers/SubscriberDirectory.vue-164-169 (1)
164-169:⚠️ Potential issue | 🟡 MinorClamp
findColumnto the supported search fields.You validate filter ids from the URL, but
findColumnis trusted as-is. A hand-edited URL can leave the component sending an unsupported column to/subscriberson every request.🛡️ Suggested fix
const searchColumns = [ { id: 'email', label: 'Email' }, { id: 'foreignKey', label: 'Foreign Key' }, { id: 'uniqueId', label: 'Unique ID' } ] +const allowedSearchColumns = new Set(searchColumns.map(({ id }) => id))if (params.has('findColumn')) { - searchColumn.value = params.get('findColumn') + const column = params.get('findColumn') + searchColumn.value = allowedSearchColumns.has(column) ? column : 'email' }🤖 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 164 - 169, The component reads params.get('findColumn') and assigns it directly to searchColumn.value, allowing an attacker to send unsupported columns; clamp this value against a whitelist before assignment by defining (or using) the supported search fields array (e.g., allowedSearchColumns or supportedSearchFields) and only set searchColumn.value if params.get('findColumn') is one of those entries, otherwise fall back to the default search column; reference the params.get('findColumn') call and the searchColumn reactive value when implementing this check.src/Controller/SubscribersController.php-125-125 (1)
125-125:⚠️ Potential issue | 🟡 MinorAssumes
subscribedListsitems have anamekey.If any list item lacks a
namekey, this will produce a PHP notice/warning. Consider defensive access.🛡️ Proposed fix
- 'lists' => implode('|', array_map(fn($list) => $list['name'], $data->subscribedLists)), + 'lists' => implode('|', array_map(fn($list) => $list['name'] ?? '', $data->subscribedLists ?? [])),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/SubscribersController.php` at line 125, The code in SubscribersController that builds 'lists' using implode('|', array_map(fn($list) => $list['name'], $data->subscribedLists)) assumes every $list has a 'name' key and can trigger notices; update the mapping to defensively access the name (e.g., check isset($list['name']) or use $list['name'] ?? null), skip or substitute empty values (filter out null/empty names) before implode, and keep the resulting expression assigned to 'lists' so missing names don't emit warnings or create empty tokens.src/Controller/SubscribersController.php-74-74 (1)
74-74:⚠️ Potential issue | 🟡 MinorPotential exception if
createdAtis null or invalid.If
$subscriber->createdAtis null or an invalid date string, theDateTimeImmutableconstructor will throw an exception. Consider adding defensive handling.🛡️ Proposed 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,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/SubscribersController.php` at line 74, The current call in SubscribersController that does '(new DateTimeImmutable($subscriber->createdAt))->format(...)' can throw if $subscriber->createdAt is null/invalid; update the code in SubscribersController to defensively handle this by checking $subscriber->createdAt before constructing DateTimeImmutable (or using DateTimeImmutable::createFromFormat / `@strtotime`) and wrap the DateTimeImmutable construction/formatting in a try/catch or conditional; if parsing fails, return a sensible fallback (e.g. null or empty string) for the 'createdAt' field so the controller does not propagate an exception.
🧹 Nitpick comments (20)
assets/vue/components/base/BaseProgressBar.vue (1)
16-25: Consider clampingvalueto the 0–100 range.If a caller passes a value outside
[0, 100], the progress bar could render with negative width or overflow. This is optional defensive coding.🛡️ Example using a computed clamped value
<script setup> +import { computed } from 'vue' + const props = defineProps({ value: { type: Number, default: 0, }, height: { type: String, default: "6px", }, }) +const clampedValue = computed(() => Math.max(0, Math.min(100, props.value))) + const wrapperStyle = computed(() => ({ height: props.height, })) </script>Then bind the width to
clampedValuein the template:-:style="{ width: value + '%' }" -:aria-valuenow="value" +:style="{ width: clampedValue + '%' }" +:aria-valuenow="clampedValue"🤖 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 16 - 25, Clamp the progress value so the rendered width cannot go outside 0–100: add a computed/clampedValue derived from the defineProps value (e.g., compute clampedValue = Math.min(100, Math.max(0, props.value || 0))) and update the template binding that currently uses value to use clampedValue instead; ensure the prop definition for value remains unchanged and reference the computed as clampedValue in BaseProgressBar.vue.apache/web-frontend.conf (1)
23-23: Avoid hardcoding a single PHP-FPM version socket.Line 23 ties this vhost to PHP 8.1 specifically. That makes deployments on PHP 8.2/8.3 fail unless this file is manually edited.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apache/web-frontend.conf` at line 23, The vhost is hardcoded to PHP 8.1 via the SetHandler socket path (SetHandler "proxy:unix:/run/php/php8.1-fpm.sock|fcgi://localhost"), causing failures on other PHP versions; update the vhost to reference a non-versioned socket or an Apache variable so deployments can pick the active PHP-FPM (for example use a generic socket like /run/php/php-fpm.sock or an Apache Define such as ${php_fpm_sock} and ensure the system provides that symlink or variable), and update any deployment/hook to manage the symlink or Define value rather than editing the vhost file directly.postcss.config.js (1)
3-4: Removeautoprefixerfrom the PostCSS configuration.This project is on Tailwind CSS v4, where
@tailwindcss/postcssincludes vendor prefixing via Lightning CSS. The separateautoprefixerplugin is redundant and can be safely removed.♻️ Proposed diff
module.exports = { plugins: [ require("@tailwindcss/postcss"), - require('autoprefixer'), ], };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@postcss.config.js` around lines 3 - 4, Remove the redundant Autoprefixer entry from the PostCSS plugin list: locate the plugins array that currently includes require("@tailwindcss/postcss") and require('autoprefixer') and delete the require('autoprefixer') item so only require("@tailwindcss/postcss") remains; ensure the resulting array syntax is valid (no leftover dangling commas) since Tailwind v4's `@tailwindcss/postcss` provides vendor prefixing via Lightning CSS.assets/vue/components/dashboard/RecentCampaignsCard.vue (1)
21-49: Avoid shipping demo rows as the runtime default.Lines 21–49 still use placeholder campaigns. Prefer an empty default (
[]) so production screens don’t silently show mock data when data wiring is missing.I can help draft a small follow-up that wires this component to the backend response shape and removes the placeholder default safely.
🤖 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 21 - 49, The prop "rows" in RecentCampaignsCard.vue currently ships demo campaign objects as the runtime default; change its default to an empty array by replacing the placeholder list with default: () => [] so production doesn't display mock data, keep the prop type as Array, and add a brief TODO or prop-doc comment referencing the backend response shape to guide wiring this prop to the API response (look for the "rows" prop definition in RecentCampaignsCard.vue).tests/Integration/Auth/LoginTest.php (1)
55-55: Use a semantic selector for login errors instead of utility classes.
.bg-red-50.border-red-200.text-red-600is styling-coupled and fragile. Prefer a stable hook (data-testid/role) or assert on the actual error text.🤖 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, Replace the fragile style-based selector in the test's assertion (the call to assertSelectorExists('.bg-red-50.border-red-200.text-red-600')) with a semantic assertion: target a stable hook such as a data-testid (e.g. data-testid="login-error") or an ARIA role (e.g. role="alert") or assert on the visible error text itself (e.g. assertSelectorTextContains or assertSee with the expected error message). Update the test to use the chosen semantic selector or text assertion so the test checks the error element semantically (data-testid or role) or its content rather than utility CSS classes.templates/base.html.twig (1)
17-17: Drop emptyclassattribute on<main>.This can be removed for cleaner markup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/base.html.twig` at line 17, Remove the empty class attribute on the <main> element in templates/base.html.twig: find the <main class=""> tag and change it to <main> (i.e., drop the redundant class="" attribute) so the markup is cleaner and free of empty attributes.assets/vue/components/dashboard/KpiGrid.vue (1)
14-14: Open TODO on backend KPI source should be tracked before release.This still ships mock KPI values. If this is not in-scope for this PR, please track it with an issue/milestone.
If helpful, I can draft the composable/API wiring for KPI fetch + loading/error states and a follow-up issue template.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/dashboard/KpiGrid.vue` at line 14, KpiGrid.vue currently ships hard-coded mock KPI values; replace that mock data with a composable that fetches real metrics from the backend: create a useKpis (or fetchKpis/getKpiMetrics) composable that returns { kpis, loading, error, refresh } and call it from the KpiGrid component (onMounted/useEffect) to populate the grid and show loading/error states instead of mocks; ensure the component handles empty/error states and exposes a refresh action; if this change is out-of-scope for this PR, open an issue/milestone tracking the TODO so the backend wiring is implemented in a follow-up.assets/router/index.js (2)
5-12: Consider configuring a base path for subdirectory deployments.
createWebHistory()is called without a base path argument. If this SPA is deployed under a subdirectory (e.g.,/admin/), routing will fail. Consider making the base path configurable:💡 Suggested improvement
export const router = createRouter({ - history: createWebHistory(), + history: createWebHistory(import.meta.env.BASE_URL || '/'), routes: [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/router/index.js` around lines 5 - 12, The router is using createWebHistory() without a base path which will break routing for subdirectory deployments; update the router initialization (the createRouter call and createWebHistory usage) to pass a configurable base path (e.g., use the app’s environment/base URL like import.meta.env.BASE_URL or a dedicated config value) so createWebHistory(base) is used; ensure the value is read once (e.g., const base = import.meta.env.BASE_URL || '/') and passed into createWebHistory(base) when constructing the exported router.
7-11: Consider handling unimplemented sidebar routes explicitly.The sidebar (AppSidebar.vue) references 7 routes without corresponding definitions in router/index.js:
/lists,/campaigns,/templates,/bounces,/analytics,/settings, and/public. These will silently redirect to the dashboard via the catch-all route. To improve user experience, either:
- Define placeholder routes with a "Coming Soon" view
- Disable the corresponding sidebar items until the features are implemented
🤖 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 - 11, The router's routes array currently only defines '/', '/subscribers' and the catch-all redirect which causes sidebar links referenced in AppSidebar.vue (/lists, /campaigns, /templates, /bounces, /analytics, /settings, /public) to silently redirect; either add explicit placeholder routes in the routes array (e.g., route objects pointing to a ComingSoonView component) for each missing path or update AppSidebar.vue to disable/remove those menu items until implemented — modify the routes array in index.js to include route objects for the missing paths (with meta.title and a ComingSoonView component) or alternatively update AppSidebar.vue to conditionally render/disable items based on a feature flag or a routesWhitelist.assets/vue/components/dashboard/KpiCard.vue (2)
16-18: Hardcoded comparison text limits reusability.The text "vs last month" is hardcoded. Consider making this configurable via a prop if KPIs may compare against different time periods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/dashboard/KpiCard.vue` around lines 16 - 18, The comparison string "vs last month" in KpiCard.vue is hardcoded; add a prop (e.g., comparisonLabel or comparisonPeriod) to the KpiCard component and replace the literal text in the template (the <p class="mb-0 text-sm" :class="trendClass"> line that currently renders "{{ change }} vs last month") with an interpolation that uses the new prop (e.g., "{{ change }} {{ comparisonLabel }}"), and provide a sensible default prop value of "vs last month" so existing usages remain unchanged.
5-10: Replace inline styles with Tailwind utility classes.The inline
styleattribute can be replaced with Tailwind's sizing utilities for consistency.♻️ Suggested refactor
<div - class="inline-flex items-center justify-center rounded-full bg-gray-100 text-gray-500 mr-2" - style="width: 40px; height: 40px;" + class="inline-flex items-center justify-center rounded-full bg-gray-100 text-gray-500 mr-2 w-10 h-10" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/dashboard/KpiCard.vue` around lines 5 - 10, The wrapper div in KpiCard.vue uses an inline style for width/height; replace the inline style on the div that wraps <BaseIcon :name="icon" /> with Tailwind sizing utilities (use w-10 h-10 to match 40px) and remove the style attribute so the element uses class-based sizing (keep existing classes like inline-flex items-center justify-center rounded-full bg-gray-100 text-gray-500 mr-2).assets/vue/components/sidebar/SidebarNavSection.vue (2)
5-9: Consider using a more stable key for v-for.Using
item.labelas the key could cause issues if labels are duplicated or change. If items have a unique identifier (likeroute), prefer that:<SidebarNavItem v-for="item in items" - :key="item.label" + :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 5 - 9, The v-for in SidebarNavSection.vue uses item.label as the key which can be unstable or duplicated; update the key on the SidebarNavItem component to use a stable unique identifier from the items (e.g., item.route or item.id) instead of item.label—locate the v-for rendering of SidebarNavItem and replace :key="item.label" with the unique property (for example :key="item.route") ensuring every item in items provides that unique field.
16-19: Remove unusedpropsassignment.The
propsconstant is declared but never referenced. The template accesseslabelanditemsdirectly.-const props = defineProps({ +defineProps({ label: String, items: { type: Array, default: () => [] }, })🤖 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 16 - 19, The declared constant props is unused; remove the unused assignment and call defineProps(...) directly (i.e., replace "const props = defineProps({...})" with just "defineProps({...})") so the component still declares label and items for the template to access; reference the props constant, defineProps, and the label/items props when making this change.assets/vue/components/sidebar/AppSidebar.vue (2)
17-22: Add aria-label to the close button.The close button should have an accessible label for screen reader users.
<button class="lg:hidden p-1.5 hover:bg-slate-100 rounded-lg text-slate-400" `@click`="closeSidebar" + aria-label="Close sidebar" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/sidebar/AppSidebar.vue` around lines 17 - 22, The close button lacks an accessible label; update the button element that calls the closeSidebar handler and contains <BaseIcon name="close" /> to include an aria-label (e.g., aria-label="Close sidebar" or an i18n string) so screen readers can describe its purpose; ensure the label text is descriptive and matches existing localization patterns if used in the component.
4-8: Add accessibility attributes to the backdrop.The backdrop overlay should be accessible to screen readers and keyboard users.
♿ Suggested improvement
<div v-if="isSidebarOpen" class="fixed inset-0 bg-slate-900/60 backdrop-blur-sm z-40 lg:hidden transition-opacity" `@click`="closeSidebar" + aria-label="Close sidebar" + role="button" + tabindex="0" + `@keydown.enter`="closeSidebar" + `@keydown.escape`="closeSidebar" ></div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/sidebar/AppSidebar.vue` around lines 4 - 8, The backdrop div used when isSidebarOpen should be keyboard- and screen-reader-accessible: add role="button", a descriptive aria-label (e.g., "Close sidebar"), and tabindex="0" to the backdrop element, keep the existing `@click`="closeSidebar", and add a keydown handler that calls closeSidebar for Enter and Escape keys; ensure the element is only present when isSidebarOpen so it is hidden from AT when closed. Target the backdrop div in AppSidebar.vue (the element using isSidebarOpen and `@click`="closeSidebar") to implement these attributes and the keydown handler.assets/vue/components/subscribers/SubscribersTable.vue (1)
5-9: This component still models campaign rows, not subscribers.The component lives under
components/subscribers/, but it rendersname/status/date/openRate/clickRateand callsstatus.toLowerCase(). That doesn’t match the subscriber shape used elsewhere in this PR (confirmed,listCount,createdAt), so wiring it up as the subscribers table will misrender rows and can crash on a missingstatus.Also applies to: 19-34, 38-40, 55-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/subscribers/SubscribersTable.vue` around lines 5 - 9, The table currently renders campaign fields (name, status, date, openRate, clickRate) and calls status.toLowerCase(), but this component should render subscriber data (email, confirmed, listCount, createdAt) so update the headers and each row binding to use the subscriber shape instead of campaign fields: replace references to name/status/date/openRate/clickRate with email/confirmed/listCount/createdAt, remove or guard the status.toLowerCase() call (use confirmed boolean display like "Yes/No" or an icon), and format createdAt into a readable date; ensure the component uses the incoming prop/variable that provides subscribers (e.g., subscribers) when iterating rows so missing status access can’t crash the render.src/Controller/SubscribersController.php (1)
24-27: Suppression annotations note.The
@SuppressWarningsannotations suggest the method has high complexity. The filter construction and pagination logic contribute to this. Consider extracting the filter building and response transformation into private helper methods to reduce cognitive load.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/SubscribersController.php` around lines 24 - 27, The method in SubscribersController.php annotated with `@SuppressWarnings` has excessive complexity due to inline filter construction and response transformation; extract the filter-building code into a private helper (e.g., buildFiltersFromRequest(Request $request): array) and move the response transformation/pagination mapping into another private helper (e.g., transformSubscriberCollection($paginator): array or formatPaginatedSubscribers). Update the original method to call these two helpers (use the helper to produce the filter array used in the repository/query and the helper to convert the paginator/result into the final response structure), and ensure any local variables used by both are passed as parameters so the controller method's cyclomatic/NPath complexity is reduced.assets/vue/components/base/BaseIcon.vue (2)
24-94: Hardcoded Tailwind classes in SVG strings limit reusability.Several icons contain hardcoded color and size classes (e.g.,
text-emerald-600,text-red-600,w-4 h-4), which override the wrapper's computed classes and prevent consistent theming via theactiveprop. Consider stripping these inline classes so icons inherit color from the wrapper'scurrentColor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/base/BaseIcon.vue` around lines 24 - 94, The icons object contains SVG strings (icons) with hardcoded Tailwind classes that override wrapper styling—remove inline size/color classes from SVGs such as addUser, removeUser, userRole, warning, template, public, list, upload, download, x, v, repeat, copy, etc., so they use stroke="currentColor" and no tailwind width/height/text-* classes; update the entries in the icons map to strip class attributes or replace them with neutral classes (or empty class attributes) so the wrapper component and the active prop can control sizing and color consistently.
12-15: Unusedsizeprop.The
sizeprop is defined with a default value but is never used in the component logic or template. Either implement size-based styling or remove the prop to avoid confusion.♻️ Suggested implementation using size prop
const wrapperClass = computed(() => { const classes = [ "inline-flex", "items-center", "justify-center", "transition-colors", ]; + // Size classes + const sizeClasses = { + sm: "w-4 h-4", + md: "w-5 h-5", + lg: "w-6 h-6", + }; + classes.push(sizeClasses[props.size] || sizeClasses.md); + if (props.active) { classes.push("text-ext-wf3"); } else { classes.push("text-slate-400 group-hover:text-slate-600"); } return classes.join(" "); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/base/BaseIcon.vue` around lines 12 - 15, The BaseIcon.vue component defines a size prop that is never used; either remove the unused prop or wire it into rendering by mapping the size values ("sm","md","lg") to actual styles/classes. Update BaseIcon.vue (the size prop on the component) to apply the size by binding it to the root element's class or inline style (e.g., compute a class like `icon--${size}` or compute width/height in a computed property and bind to :class or :style), or remove the size prop entirely if sizing is handled externally.tests/Unit/Controller/AuthControllerTest.php (1)
184-189: SamewithConsecutive()deprecation applies here.Apply the same refactor pattern as suggested for the login test.
🤖 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 184 - 189, The withConsecutive() call on the $session->expects(...)->method('remove') is deprecated; replace it with a single expectation that uses willReturnCallback to inspect each invocation's argument and assert the sequence ['auth_token','auth_id'] (use a local counter/closure to track call index and assert the passed key matches the expected value) while keeping the expectation count exact (exactly(2)). Locate the $session->expects(...)->method('remove') block in AuthControllerTest and change it to use willReturnCallback with an internal counter and assertion checks for the two expected keys.
Comment on lines
+8
to
+14
| <link rel="stylesheet" href="{{ asset('build/app.css') }}"> | ||
| {# Add your own custom CSS later if needed #} | ||
| {{ encore_entry_link_tags('app') }} | ||
| {{ encore_entry_link_tags('styles') }} | ||
|
|
||
| {% block javascripts %} | ||
| {{ encore_entry_script_tags('app') }} | ||
| {% endblock %} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for mixed Encore + direct build asset inclusion in Twig templates.
rg -n "encore_entry_(link|script)_tags|asset\\('build/(app|runtime)\\.(css|js)'\\)" templatesRepository: phpList/web-frontend
Length of output: 463
🏁 Script executed:
cat -n templates/base.html.twig | head -30Repository: phpList/web-frontend
Length of output: 939
Consolidate asset loading to use only Encore entry tags to avoid double-loading.
The template loads CSS and JS bundles twice: direct asset('build/...') includes on lines 8, 21–22 and Encore entry tags on lines 10, 13. This causes duplicate CSS and bootstraps the app twice, risking initialization duplication and listener conflicts.
Remove the direct asset calls and rely solely on encore_entry_link_tags('styles') and encore_entry_script_tags('app'):
Proposed fix
- <link rel="stylesheet" href="{{ asset('build/app.css') }}">
{# Add your own custom CSS later if needed #}
{{ encore_entry_link_tags('styles') }}-<script src="{{ asset('build/runtime.js') }}"></script>
-<script src="{{ asset('build/app.js') }}"></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
| <link rel="stylesheet" href="{{ asset('build/app.css') }}"> | |
| {# Add your own custom CSS later if needed #} | |
| {{ encore_entry_link_tags('app') }} | |
| {{ encore_entry_link_tags('styles') }} | |
| {% block javascripts %} | |
| {{ encore_entry_script_tags('app') }} | |
| {% endblock %} | |
| {# Add your own custom CSS later if needed #} | |
| {{ encore_entry_link_tags('styles') }} | |
| {% block javascripts %} | |
| {{ encore_entry_script_tags('app') }} | |
| {% endblock %} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/base.html.twig` around lines 8 - 14, Remove the direct asset
includes and rely only on Webpack Encore entry tags to avoid duplicate loading:
delete the asset('build/app.css') and any other direct asset(...) or link/script
tags that point to build files, and keep only {{
encore_entry_link_tags('styles') }} and the {% block javascripts %} containing
{{ encore_entry_script_tags('app') }}; update the template around the existing
encore_entry_link_tags('styles') and encore_entry_script_tags('app') calls so
the stylesheet and JS are each included once and no manual <link> or <script>
referencing build/* remains.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Controller/AuthController.php (2)
65-70:⚠️ Potential issue | 🟠 MajorHarden logout before this ships.
A cross-site GET can trigger this route,
auth_expiry_datesurvives the logout, and a thrownAuthClient::logout()will currently turn a local signout into a 500. Make this POST+CSRF, clear all auth session keys, and treat the upstream logout as best-effort.🔒 Proposed fix
- #[Route('/logout', name: 'logout')] + #[Route('/logout', name: 'logout', methods: ['POST'])] public function logout(Request $request): Response { - $request->getSession()->remove('auth_token'); - $request->getSession()->remove('auth_id'); - $this->authClient->logout(); + if (!$this->isCsrfTokenValid('logout', (string) $request->request->get('_token'))) { + throw $this->createAccessDeniedException(); + } + + $session = $request->getSession(); + $session->remove('auth_token'); + $session->remove('auth_expiry_date'); + $session->remove('auth_id'); + + try { + $this->authClient->logout(); + } catch (Exception|GuzzleException $e) { + // log and continue; the local session is already cleared + } return $this->redirectToRoute('login'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/AuthController.php` around lines 65 - 70, Change the logout action to require POST+CSRF and make upstream logout best-effort: update the attribute on the logout method to restrict to POST (e.g. #[Route('/logout', name: 'logout', methods: ['POST'])]), validate the CSRF token from $request->request->get('_token') via isCsrfTokenValid('logout', ...), clear all auth-related session keys (remove 'auth_token', 'auth_id', 'auth_expiry_date' and/or call $request->getSession()->invalidate() if appropriate) instead of leaving auth_expiry_date behind, and wrap the $this->authClient->logout() call in a try/catch so any exception is swallowed/logged but does not turn signout into a 500; finally return an appropriate redirect/response.
53-56:⚠️ Potential issue | 🟠 MajorDon't send raw auth/client error text back to the login page.
This leaks upstream details straight into the UI. Keep the response generic here and log the exception server-side instead.
🔧 Proposed fix
- } catch (Exception $e) { - $error = 'Invalid credentials or server error: ' . $e->getMessage(); - } catch (GuzzleException $e) { - $error = 'Invalid credentials or server error: ' . $e->getMessage(); + } catch (Exception|GuzzleException $e) { + $error = 'Invalid credentials or server error.'; }🤖 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 - 56, The catch blocks in AuthController currently append raw exception messages to $error (see the catch (Exception $e) and catch (GuzzleException $e) handlers); change these to set a generic user-facing message like "Invalid credentials or server error." and move the detailed exception information to server-side logging (use the controller's logger or error_log to record $e->getMessage() and stack trace). Ensure both catch blocks behave the same (generic $error for the UI, detailed log entry for diagnostics) and avoid exposing $e->getMessage() to the view.
♻️ Duplicate comments (1)
assets/vue/components/subscribers/SubscriberModal.vue (1)
13-13:⚠️ Potential issue | 🔴 CriticalGuard subscriber access and reset modal state between openings.
subscriberstarts asnull, so the current header expression can break at render time, and stale details can leak across reopen cycles because state is not cleared before fetch/close.Suggested patch
- Subscriber Details ID: {{ subscriber.id ?? '' }} + Subscriber Details ID: {{ subscriber?.id ?? props.subscriberId ?? '—' }} const fetchSubscriberDetails = async () => { + subscriber.value = null loading.value = true error.value = null try { subscriber.value = await subscribersClient.getSubscriber(props.subscriberId) @@ const close = () => { + subscriber.value = null + error.value = null + formData.value = { + email: '', + confirmed: false, + blacklisted: false, + htmlEmail: false, + disabled: false + } emit('close') }Also applies to: 164-166, 199-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/subscribers/SubscriberModal.vue` at line 13, Header and template access crash because subscriber can be null and stale data leaks between opens; guard template reads (use v-if or null-coalescing around subscriber.id in the header and other bindings at lines referenced) and reset the modal state by clearing the component data property subscriber before fetching new details and on close. Update the modal-opening/fetching flow (the methods that fetch subscriber details and close the modal — e.g., your fetchSubscriber/openModal/closeModal handlers) to set subscriber = null before starting a request, set subscriber only after a successful fetch, and ensure closeModal clears subscriber and any related state so stale details cannot persist across reopenings.
🧹 Nitpick comments (3)
src/EventSubscriber/AuthGateSubscriber.php (1)
63-70: Nit: redundant exact-match check for/login.
str_starts_with($path, '/login')already covers the exact match case$path === '/login'. Not a big deal — just a tiny simplification if you're ever in the neighborhood.Optional cleanup
// Public login route - if ($path === '/login' || str_starts_with($path, '/login')) { + if (str_starts_with($path, '/login')) { return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EventSubscriber/AuthGateSubscriber.php` around lines 63 - 70, The isPublicPath(Request $request) function contains a redundant exact-match check ($path === '/login') because str_starts_with($path, '/login') already covers that case; simplify by removing the "$path === '/login'" branch and rely solely on str_starts_with($path, '/login') (or return its boolean result) when determining the public login route.assets/vue/components/subscribers/SubscriberDirectory.vue (1)
94-94: Clear the debounce timer on unmount.Small lifecycle cleanup: pending timer can still fire after unmount.
Suggested patch
-import {inject, onMounted, ref} from 'vue' +import { inject, onBeforeUnmount, onMounted, ref } from 'vue' @@ let searchTimeout = null + +onBeforeUnmount(() => { + if (searchTimeout) clearTimeout(searchTimeout) +})Also applies to: 122-122, 218-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/subscribers/SubscriberDirectory.vue` at line 94, The component SubscriberDirectory sets debounce timers via setTimeout in its setup (see the setTimeout-based debounce usages around the onMounted usage and lines referenced) but never clears them on unmount; import onUnmounted from 'vue' and add an onUnmounted handler that calls clearTimeout for each timer variable used (e.g., debounceTimer / searchTimer / whatever names you used for the timers in setup), or track and clear all active timeout IDs created at lines ~94, ~122 and ~218-225 so pending timers cannot fire after unmount.tests/Unit/Controller/AuthControllerTest.php (1)
186-187: Tighten this redirect assertion.Checking for
/will pass for almost any redirect, so it won't catch a wrong target route. Assert the home route explicitly here.🎯 Proposed fix
- $this->assertStringContainsString('/', $response->getTargetUrl()); + $this->assertStringContainsString('mocked-route-to-home', $response->getTargetUrl());🤖 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 186 - 187, The test currently uses a loose check ($response->getTargetUrl()) that only asserts '/' is contained; tighten it by asserting the redirect target equals the application's home URL exactly: replace the assertStringContainsString('/', $response->getTargetUrl()) with an equality assertion comparing $response->getTargetUrl() to the canonical home URL (e.g. use route('home') or url('/') depending on your app) inside AuthControllerTest so the test fails if the redirect goes to the wrong route.
🤖 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/subscribers/SubscriberDirectory.vue`:
- Around line 168-170: Whitelist the incoming URL-driven findColumn before
assigning or forwarding it: validate params.get('findColumn') against your known
column list (e.g., an allowedColumns constant or the same options used to
populate the UI dropdown) and only set searchColumn.value if it matches;
otherwise ignore or fallback to a default. Apply the same whitelist check where
findColumn is read/forwarded elsewhere (the other block around lines 190–193) so
no arbitrary URL value is sent upstream or used to initialize state.
- Around line 180-214: fetchSubscribers can be called concurrently and older
fetches can overwrite newer state; fix this by adding an AbortController per
in-flight request: keep a top-level variable (e.g., currentFetchController) and
in fetchSubscribers call currentFetchController?.abort() then create a new
AbortController and pass its signal to fetch; in the catch block ignore
DOMException/AbortError so aborted requests don't log or mutate state; only set
subscribers.value, pagination.value and call updateUrl(afterId) when the
response completes successfully (i.e., not aborted); also abort
currentFetchController when the component is unmounted to avoid leaks.
In `@assets/vue/components/subscribers/SubscriberModal.vue`:
- Around line 158-162: The watcher currently only watches props.isOpen so when
props.subscriberId changes while the modal is open the form is not refetched;
update the watcher on SubscriberModal.vue to watch both props.isOpen and
props.subscriberId (e.g., watch an array or two getters) and call
fetchSubscriberDetails() whenever props.isOpen is true and props.subscriberId is
present so the form is reloaded for the new ID.
In `@src/Controller/AuthController.php`:
- Around line 75-78: The about() action assumes
$this->authClient->getSessionUser() always returns a User and never throws;
change it to catch errors and handle missing sessions: call getSessionUser()
inside a try/catch, check the result for null/false, and return a JsonResponse
with a 401 (unauthenticated) when there is no session user and a 503 (or
appropriate error) if an exception occurs contacting the auth backend; update
the about() method to log the error and return controlled responses instead of
letting exceptions bubble up from getSessionUser().
In `@src/EventSubscriber/AuthGateSubscriber.php`:
- Around line 45-61: In onKernelRequest (AuthGateSubscriber::onKernelRequest)
add a guard that checks $request->hasSession() before calling
$request->getSession(); if hasSession() is false simply return (like
ApiSessionListener does) to avoid calling getSession() on stateless requests —
locate the onKernelRequest method and insert an early return when
!$request->hasSession() before the existing $session = $request->getSession()
usage so the subsequent has('auth_token') check only runs when a session exists.
---
Outside diff comments:
In `@src/Controller/AuthController.php`:
- Around line 65-70: Change the logout action to require POST+CSRF and make
upstream logout best-effort: update the attribute on the logout method to
restrict to POST (e.g. #[Route('/logout', name: 'logout', methods: ['POST'])]),
validate the CSRF token from $request->request->get('_token') via
isCsrfTokenValid('logout', ...), clear all auth-related session keys (remove
'auth_token', 'auth_id', 'auth_expiry_date' and/or call
$request->getSession()->invalidate() if appropriate) instead of leaving
auth_expiry_date behind, and wrap the $this->authClient->logout() call in a
try/catch so any exception is swallowed/logged but does not turn signout into a
500; finally return an appropriate redirect/response.
- Around line 53-56: The catch blocks in AuthController currently append raw
exception messages to $error (see the catch (Exception $e) and catch
(GuzzleException $e) handlers); change these to set a generic user-facing
message like "Invalid credentials or server error." and move the detailed
exception information to server-side logging (use the controller's logger or
error_log to record $e->getMessage() and stack trace). Ensure both catch blocks
behave the same (generic $error for the UI, detailed log entry for diagnostics)
and avoid exposing $e->getMessage() to the view.
---
Duplicate comments:
In `@assets/vue/components/subscribers/SubscriberModal.vue`:
- Line 13: Header and template access crash because subscriber can be null and
stale data leaks between opens; guard template reads (use v-if or
null-coalescing around subscriber.id in the header and other bindings at lines
referenced) and reset the modal state by clearing the component data property
subscriber before fetching new details and on close. Update the
modal-opening/fetching flow (the methods that fetch subscriber details and close
the modal — e.g., your fetchSubscriber/openModal/closeModal handlers) to set
subscriber = null before starting a request, set subscriber only after a
successful fetch, and ensure closeModal clears subscriber and any related state
so stale details cannot persist across reopenings.
---
Nitpick comments:
In `@assets/vue/components/subscribers/SubscriberDirectory.vue`:
- Line 94: The component SubscriberDirectory sets debounce timers via setTimeout
in its setup (see the setTimeout-based debounce usages around the onMounted
usage and lines referenced) but never clears them on unmount; import onUnmounted
from 'vue' and add an onUnmounted handler that calls clearTimeout for each timer
variable used (e.g., debounceTimer / searchTimer / whatever names you used for
the timers in setup), or track and clear all active timeout IDs created at lines
~94, ~122 and ~218-225 so pending timers cannot fire after unmount.
In `@src/EventSubscriber/AuthGateSubscriber.php`:
- Around line 63-70: The isPublicPath(Request $request) function contains a
redundant exact-match check ($path === '/login') because str_starts_with($path,
'/login') already covers that case; simplify by removing the "$path ===
'/login'" branch and rely solely on str_starts_with($path, '/login') (or return
its boolean result) when determining the public login route.
In `@tests/Unit/Controller/AuthControllerTest.php`:
- Around line 186-187: The test currently uses a loose check
($response->getTargetUrl()) that only asserts '/' is contained; tighten it by
asserting the redirect target equals the application's home URL exactly: replace
the assertStringContainsString('/', $response->getTargetUrl()) with an equality
assertion comparing $response->getTargetUrl() to the canonical home URL (e.g.
use route('home') or url('/') depending on your app) inside AuthControllerTest
so the test fails if the redirect goes to the wrong route.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab5ff3e8-563c-42c7-9cc8-ac83be91b5be
📒 Files selected for processing (7)
.coderabbit.yaml.github/PULL_REQUEST_TEMPLATE.mdassets/vue/components/subscribers/SubscriberDirectory.vueassets/vue/components/subscribers/SubscriberModal.vuesrc/Controller/AuthController.phpsrc/EventSubscriber/AuthGateSubscriber.phptests/Unit/Controller/AuthControllerTest.php
✅ Files skipped from review due to trivial changes (1)
- .github/PULL_REQUEST_TEMPLATE.md
Comment on lines
+45
to
+61
| public function onKernelRequest(RequestEvent $event): void | ||
| { | ||
| if (!$event->isMainRequest()) { | ||
| return; | ||
| } | ||
|
|
||
| $request = $event->getRequest(); | ||
| if ($this->isPublicPath($request)) { | ||
| return; | ||
| } | ||
|
|
||
| $session = $request->getSession(); | ||
| if (!$session->has('auth_token')) { | ||
| $loginUrl = $this->urlGenerator->generate('login'); | ||
| $event->setResponse(new RedirectResponse($loginUrl)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a hasSession() guard before calling getSession().
Looking at how ApiSessionListener handles this (it checks $request->hasSession() before proceeding), this subscriber should do the same. Stateless requests or edge-case configurations could throw if getSession() is called without an active session.
Proposed fix
public function onKernelRequest(RequestEvent $event): void
{
if (!$event->isMainRequest()) {
return;
}
$request = $event->getRequest();
if ($this->isPublicPath($request)) {
return;
}
+ if (!$request->hasSession()) {
+ return;
+ }
+
$session = $request->getSession();
if (!$session->has('auth_token')) {
$loginUrl = $this->urlGenerator->generate('login');
$event->setResponse(new RedirectResponse($loginUrl));
}
}📝 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
| public function onKernelRequest(RequestEvent $event): void | |
| { | |
| if (!$event->isMainRequest()) { | |
| return; | |
| } | |
| $request = $event->getRequest(); | |
| if ($this->isPublicPath($request)) { | |
| return; | |
| } | |
| $session = $request->getSession(); | |
| if (!$session->has('auth_token')) { | |
| $loginUrl = $this->urlGenerator->generate('login'); | |
| $event->setResponse(new RedirectResponse($loginUrl)); | |
| } | |
| } | |
| public function onKernelRequest(RequestEvent $event): void | |
| { | |
| if (!$event->isMainRequest()) { | |
| return; | |
| } | |
| $request = $event->getRequest(); | |
| if ($this->isPublicPath($request)) { | |
| return; | |
| } | |
| if (!$request->hasSession()) { | |
| return; | |
| } | |
| $session = $request->getSession(); | |
| if (!$session->has('auth_token')) { | |
| $loginUrl = $this->urlGenerator->generate('login'); | |
| $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/AuthGateSubscriber.php` around lines 45 - 61, In
onKernelRequest (AuthGateSubscriber::onKernelRequest) add a guard that checks
$request->hasSession() before calling $request->getSession(); if hasSession() is
false simply return (like ApiSessionListener does) to avoid calling getSession()
on stateless requests — locate the onKernelRequest method and insert an early
return when !$request->hasSession() before the existing $session =
$request->getSession() usage so the subsequent has('auth_token') check only runs
when a session exists.
TatevikGr
force-pushed
the
feat/subscribers
branch
from
be3be17 to
4954060
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
assets/vue/components/subscribers/SubscriberDirectory.vue (2)
180-214:⚠️ Potential issue | 🟠 MajorCancel in-flight fetches to prevent stale response overwrites.
fetchSubscriberscan run concurrently (search/filter/pagination/import refresh). Without cancellation, an older response can overwrite newer UI state.Suggested patch
-import {inject, onMounted, ref} from 'vue' +import {inject, onMounted, onUnmounted, ref} from 'vue' @@ let searchTimeout = null +let currentFetchController = null @@ const fetchSubscribers = async (afterId = null) => { + currentFetchController?.abort() + currentFetchController = new AbortController() + const { signal } = currentFetchController + const url = new URL('/subscribers', window.location.origin) @@ try { const response = await fetch(url, { - headers: { Accept: 'application/json', 'X-Requested-With': 'XMLHttpRequest' } + headers: { Accept: 'application/json', 'X-Requested-With': 'XMLHttpRequest' }, + signal }) @@ } catch (error) { + if (error?.name === 'AbortError') return console.error('Failed to fetch subscribers:', error) } } @@ onMounted(() => { currentFilter.value = getFilterFromUrl() fetchSubscribers() }) + +onUnmounted(() => { + if (searchTimeout) clearTimeout(searchTimeout) + currentFetchController?.abort() +})#!/bin/bash # Verify cancellation and cleanup guards exist in SubscriberDirectory.vue rg -n 'AbortController|signal\s*:|abort\(|onUnmounted|clearTimeout\(searchTimeout\)' assets/vue/components/subscribers/SubscriberDirectory.vueAlso applies to: 175-178, 218-225
🤖 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 180 - 214, fetchSubscribers can produce stale UI updates when multiple requests overlap; introduce an AbortController stored in a module-scoped ref (e.g., currentFetchController) and before starting a new fetch call abort and replace the previous controller, pass controller.signal into fetch, and in the catch block ignore AbortError while still logging other errors; also ensure onUnmounted aborts any in-flight controller to avoid leaks. Update references in fetchSubscribers and any callers (search/filter/pagination/import refresh handlers) to rely on this single controller lifecycle and keep updateUrl/unrelated logic unchanged.
168-170:⚠️ Potential issue | 🟠 MajorWhitelist URL-derived
findColumnbefore propagating it.Line 168 currently trusts a raw query param and that value is later forwarded on Line 191 and Line 301. This bypasses the dropdown constraints and can send unsupported columns upstream.
Suggested patch
const searchColumns = [ { id: 'email', label: 'Email' }, { id: 'foreignKey', label: 'Foreign Key' }, { id: 'uniqueId', label: 'Unique ID' } ] +const allowedSearchColumns = new Set(searchColumns.map(col => col.id)) +const normalizeSearchColumn = (value) => + allowedSearchColumns.has(value) ? value : 'email' @@ if (params.has('findColumn')) { - searchColumn.value = params.get('findColumn') + searchColumn.value = normalizeSearchColumn(params.get('findColumn')) } @@ if (searchQuery.value) { - url.searchParams.set('findColumn', searchColumn.value) + url.searchParams.set('findColumn', normalizeSearchColumn(searchColumn.value)) url.searchParams.set('findValue', searchQuery.value) } @@ if (searchQuery.value) { - params.set('findColumn', searchColumn.value) + params.set('findColumn', normalizeSearchColumn(searchColumn.value)) params.set('findValue', searchQuery.value) }Also applies to: 143-145, 190-193, 300-302
🤖 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 168 - 170, The code currently assigns the raw URL param to searchColumn (searchColumn.value = params.get('findColumn')) and later forwards it; instead, validate the param against the component's allowed column list (e.g., the dropdown's options/array used by the column selector such as columnOptions or similar) before assigning: read params.get('findColumn'), check it exists in the allowedColumns array, and only then set searchColumn.value (otherwise leave default or set to a safe fallback). Apply the same whitelist check wherever params.get('findColumn') is read (the other occurrences that set searchColumn or propagate its value) so only supported columns from the dropdown can be propagated upstream.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/System/ApplicationBundle/PhpListApplicationBundleTest.php`:
- Around line 38-45: The test currently follows redirects so it asserts the
login page content instead of the redirect response; update the call to
$this->httpClient->get (in PhpListApplicationBundleTest) to disable redirects by
passing 'allow_redirects' => false, then assert the response status is 302
(replace or keep the assertSame on getStatusCode) and assert the Location header
points to the login URL (check the 'Location' header on the response). Remove or
move the assertStringContainsString check for the login page content into a
dedicated login-page test so this test only verifies the redirect/authorization
behavior.
---
Duplicate comments:
In `@assets/vue/components/subscribers/SubscriberDirectory.vue`:
- Around line 180-214: fetchSubscribers can produce stale UI updates when
multiple requests overlap; introduce an AbortController stored in a
module-scoped ref (e.g., currentFetchController) and before starting a new fetch
call abort and replace the previous controller, pass controller.signal into
fetch, and in the catch block ignore AbortError while still logging other
errors; also ensure onUnmounted aborts any in-flight controller to avoid leaks.
Update references in fetchSubscribers and any callers
(search/filter/pagination/import refresh handlers) to rely on this single
controller lifecycle and keep updateUrl/unrelated logic unchanged.
- Around line 168-170: The code currently assigns the raw URL param to
searchColumn (searchColumn.value = params.get('findColumn')) and later forwards
it; instead, validate the param against the component's allowed column list
(e.g., the dropdown's options/array used by the column selector such as
columnOptions or similar) before assigning: read params.get('findColumn'), check
it exists in the allowedColumns array, and only then set searchColumn.value
(otherwise leave default or set to a safe fallback). Apply the same whitelist
check wherever params.get('findColumn') is read (the other occurrences that set
searchColumn or propagate its value) so only supported columns from the dropdown
can be propagated upstream.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c314a45c-0e4e-4229-b34a-0362d1f72808
📒 Files selected for processing (6)
assets/vue/components/subscribers/SubscriberDirectory.vueassets/vue/components/subscribers/SubscriberModal.vuesrc/Controller/AuthController.phpsrc/EventSubscriber/AuthGateSubscriber.phptests/System/ApplicationBundle/PhpListApplicationBundleTest.phptests/Unit/Controller/AuthControllerTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Controller/AuthController.php
- tests/Unit/Controller/AuthControllerTest.php
- assets/vue/components/subscribers/SubscriberModal.vue
- src/EventSubscriber/AuthGateSubscriber.php
Comment on lines
+38
to
45
| $response = $this->httpClient->get('/', [ | ||
| 'base_uri' => $this->getBaseUrl(), | ||
| ]); | ||
|
|
||
| self::assertSame(200, $response->getStatusCode()); | ||
| self::assertStringContainsString( | ||
| 'This page has been intentionally left empty.', | ||
| 'Access is restricted to authorised administrators only.', | ||
| $response->getBody()->getContents() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify test request options and assertions
rg -n -C3 "testHomepageReturnsSuccess|allow_redirects|get\('/'|assertSame\(" tests/System/ApplicationBundle/PhpListApplicationBundleTest.php
# Verify access control for root path
rg -n -C3 "access_control|\^/|ROLE_ADMIN|PUBLIC_ACCESS" config/packages/security.yaml
# Verify authenticator entry point behavior
rg -n -C3 "function start\(|RedirectResponse|generate\('login'\)" src/Security/SessionAuthenticator.phpRepository: phpList/web-frontend
Length of output: 1651
Assert the redirect response itself, not the login page it leads to.
Without allow_redirects => false, Guzzle automatically follows the 302 redirect to /login and your assertion passes on the login page content. To properly verify authorization, assert the redirect response (302 status + Location header) directly, then keep login-page content checks in a dedicated login test.
Suggested fix
public function testHomepageReturnsSuccess(): void
{
$this->startSymfonyServer();
$response = $this->httpClient->get('/', [
'base_uri' => $this->getBaseUrl(),
+ 'allow_redirects' => false,
]);
- self::assertSame(200, $response->getStatusCode());
- self::assertStringContainsString(
- 'Access is restricted to authorised administrators only.',
- $response->getBody()->getContents()
- );
+ self::assertSame(302, $response->getStatusCode());
+ self::assertSame('/login', parse_url($response->getHeaderLine('Location'), PHP_URL_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
| $response = $this->httpClient->get('/', [ | |
| 'base_uri' => $this->getBaseUrl(), | |
| ]); | |
| self::assertSame(200, $response->getStatusCode()); | |
| self::assertStringContainsString( | |
| 'This page has been intentionally left empty.', | |
| 'Access is restricted to authorised administrators only.', | |
| $response->getBody()->getContents() | |
| public function testHomepageReturnsSuccess(): void | |
| { | |
| $this->startSymfonyServer(); | |
| $response = $this->httpClient->get('/', [ | |
| 'base_uri' => $this->getBaseUrl(), | |
| 'allow_redirects' => false, | |
| ]); | |
| self::assertSame(302, $response->getStatusCode()); | |
| self::assertSame('/login', parse_url($response->getHeaderLine('Location'), PHP_URL_PATH)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/System/ApplicationBundle/PhpListApplicationBundleTest.php` around lines
38 - 45, The test currently follows redirects so it asserts the login page
content instead of the redirect response; update the call to
$this->httpClient->get (in PhpListApplicationBundleTest) to disable redirects by
passing 'allow_redirects' => false, then assert the response status is 302
(replace or keep the assertSame on getStatusCode) and assert the Location header
points to the login URL (check the 'Location' header on the response). Remove or
move the assertStringContainsString check for the login page content into a
dedicated login-page test so this test only verifies the redirect/authorization
behavior.
4954060 to
987580e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
assets/vue/components/subscribers/SubscriberModal.vue (2)
158-162:⚠️ Potential issue | 🟠 MajorWatch
subscriberIdtogether withisOpen.Right now, Line 158 only watches open/close state. If
subscriberIdchanges while open, the form can stay on old data and save against the new ID.Suggested patch
-watch(() => props.isOpen, (newValue) => { - if (newValue && props.subscriberId) { - fetchSubscriberDetails() - } -}) +watch( + () => [props.isOpen, props.subscriberId], + ([isOpen, subscriberId]) => { + if (isOpen && subscriberId) { + fetchSubscriberDetails() + } + } +)🤖 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 158 - 162, The watcher only observes props.isOpen so when props.subscriberId changes while the modal is open the form isn't refreshed; update the watcher to observe both props.isOpen and props.subscriberId (or use a computed tuple like () => [props.isOpen, props.subscriberId]) and call fetchSubscriberDetails() when the modal is open and subscriberId is present; make sure the watcher callback logic still guards for newValue.open state and a valid props.subscriberId before fetching to avoid unnecessary calls.
12-14:⚠️ Potential issue | 🔴 CriticalGuard and reset subscriber state to avoid null/stale rendering.
Line 13 dereferences
subscriber.idwhilesubscriberstarts asnull. Also, not clearingsubscriberbefore fetch/close can show stale details during reloads.Suggested patch
- Subscriber Details ID: {{ subscriber.id ?? '' }} + Subscriber Details ID: {{ subscriber?.id ?? props.subscriberId ?? '—' }}const fetchSubscriberDetails = async () => { + subscriber.value = null loading.value = true error.value = nullconst close = () => { + subscriber.value = null + error.value = null emit('close') }Also applies to: 164-166, 199-200
🤖 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 12 - 14, Template dereferences subscriber.id while the component's data property subscriber can be null and stale between loads; update the template to safely access the id (e.g., use a null-safe expression like subscriber?.id or a computed/displayId) and, in the component logic, explicitly reset subscriber to null when starting a fetch and when closing the modal (adjust the methods that load/close the modal—e.g., fetchSubscriber, openModal/onOpen, closeModal/onClose—to set this.subscriber = null before/after requests) so stale details are never rendered.assets/vue/components/subscribers/SubscriberDirectory.vue (2)
180-214:⚠️ Potential issue | 🟠 MajorAdd request cancellation to prevent stale overwrite races.
Lines 180-214 still allow overlapping fetches; older responses can land later and overwrite fresher UI state.
Suggested patch
+import {inject, onMounted, onBeforeUnmount, ref} from 'vue' @@ let searchTimeout = null +let fetchController = null @@ const fetchSubscribers = async (afterId = null) => { + fetchController?.abort() + fetchController = new AbortController() + const { signal } = fetchController + const url = new URL('/subscribers', window.location.origin) @@ const response = await fetch(url, { - headers: { Accept: 'application/json', 'X-Requested-With': 'XMLHttpRequest' } + headers: { Accept: 'application/json', 'X-Requested-With': 'XMLHttpRequest' }, + signal }) @@ } catch (error) { + if (error?.name === 'AbortError') return console.error('Failed to fetch subscribers:', error) } } + +onBeforeUnmount(() => { + fetchController?.abort() + if (searchTimeout) clearTimeout(searchTimeout) +})🤖 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 180 - 214, fetchSubscribers can start overlapping fetches that let older responses overwrite newer state; fix by adding an AbortController stored outside the function (e.g., a module- or component-scoped variable/ref) and before each new call abort the previous controller and create a fresh one, pass its signal into fetch, and ignore/early-return on AbortError so only the latest response updates subscribers.value, pagination.value and calls updateUrl; ensure the controller variable is referenced in fetchSubscribers and that any error handling distinguishes aborts from real network errors.
168-170:⚠️ Potential issue | 🟠 MajorWhitelist URL
findColumnbefore state assignment and request forwarding.Line 169 accepts any
findColumnfrom URL params and Line 191 forwards it unchanged. That bypasses your dropdown constraints and can send invalid query values upstream.Suggested patch
const searchColumns = [ { id: 'email', label: 'Email' }, { id: 'foreignKey', label: 'Foreign Key' }, { id: 'uniqueId', label: 'Unique ID' } ] +const allowedSearchColumns = new Set(searchColumns.map((c) => c.id)) @@ if (params.has('findColumn')) { - searchColumn.value = params.get('findColumn') + const candidate = params.get('findColumn') + searchColumn.value = allowedSearchColumns.has(candidate) ? candidate : 'email' } @@ if (searchQuery.value) { - url.searchParams.set('findColumn', searchColumn.value) + const column = allowedSearchColumns.has(searchColumn.value) ? searchColumn.value : 'email' + url.searchParams.set('findColumn', column) url.searchParams.set('findValue', searchQuery.value) }Also applies to: 190-193
🤖 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 168 - 170, Whitelist and validate the URL param "findColumn" before assigning it to searchColumn or forwarding it in requests: in SubscriberDirectory.vue define an allowedColumns array (the same set your dropdown uses), check params.get('findColumn') against that list and only assign searchColumn.value when it matches, otherwise use a safe default; when building outbound requests (the code that adds findColumn to query params) use the validated/whitelisted value (not the raw URL param) so invalid or malicious values are never forwarded.src/Controller/AuthController.php (1)
75-79:⚠️ Potential issue | 🟠 MajorHandle
/admin-aboutbackend/session failures explicitly.Line 78 still assumes
getSessionUser()always returns a user and never fails. This can bubble to a 500 instead of controlled 401/503 responses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/AuthController.php` around lines 75 - 79, The about() action in AuthController currently assumes authClient->getSessionUser() always returns a valid user; update it to handle null/failed responses and exceptions by checking the result of getSessionUser(), returning a JsonResponse with 401 when no session/user is present, and catching any exceptions from authClient->getSessionUser() to return a 503 with an error payload; ensure you reference the about() method and authClient->getSessionUser() and avoid throwing unhandled exceptions so the endpoint always returns a controlled 401/503 response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Controller/AuthController.php`:
- Around line 68-70: The logout sequence in AuthController currently removes
'auth_token' and 'auth_id' but leaves 'auth_expiry_date' in the session, which
can cause stale-session behavior; update the logout logic (the block that calls
$this->authClient->logout()) to also remove 'auth_expiry_date' from the session
(i.e., call $request->getSession()->remove('auth_expiry_date') in the same place
where 'auth_token' and 'auth_id' are removed).
In `@src/Controller/SubscribersController.php`:
- Line 40: The $limit pulled from $request->query->get('limit', 10) must be
capped to prevent oversized fetch/export requests: introduce a MAX_LIMIT
constant (e.g. private const MAX_LIMIT = 1000) in SubscribersController and
replace both instances where $limit is computed ($limit = max(1, (int)
$request->query->get('limit', 10))) with a capped expression (use
min(self::MAX_LIMIT, max(1, (int) $request->query->get('limit', 10)))) so both
the list/export code paths use the same upper bound.
- Line 92: The Route attribute on the export endpoint uses a plain string for
methods (#[Route('/export', name: 'export', methods: 'GET')]) which doesn't
match Symfony's attribute API; update the attribute to use an array for methods
(methods: ['GET']) on the export route in SubscribersController (the
#[Route(..., name: 'export', ...)] attribute) so it matches the other routes and
Symfony's expected format.
- Around line 126-139: The CSV export in SubscribersController is writing
user-controlled fields directly into $row (keys 'email', 'uniqueId', 'lists')
and is vulnerable to spreadsheet formula injection; fix by sanitizing these
fields before fputcsv — implement or call a helper like sanitizeCsvValue($value)
inside the code that builds $row (or when assigning 'email', 'uniqueId', and the
imploded 'lists') to prefix or neutralize any value that begins with =, +, -, or
@ (e.g., prepend a single quote or safe character) and ensure the sanitized
output preserves non-formula content; update SubscribersController where $row is
constructed so all potentially user-supplied columns use the sanitizer.
---
Duplicate comments:
In `@assets/vue/components/subscribers/SubscriberDirectory.vue`:
- Around line 180-214: fetchSubscribers can start overlapping fetches that let
older responses overwrite newer state; fix by adding an AbortController stored
outside the function (e.g., a module- or component-scoped variable/ref) and
before each new call abort the previous controller and create a fresh one, pass
its signal into fetch, and ignore/early-return on AbortError so only the latest
response updates subscribers.value, pagination.value and calls updateUrl; ensure
the controller variable is referenced in fetchSubscribers and that any error
handling distinguishes aborts from real network errors.
- Around line 168-170: Whitelist and validate the URL param "findColumn" before
assigning it to searchColumn or forwarding it in requests: in
SubscriberDirectory.vue define an allowedColumns array (the same set your
dropdown uses), check params.get('findColumn') against that list and only assign
searchColumn.value when it matches, otherwise use a safe default; when building
outbound requests (the code that adds findColumn to query params) use the
validated/whitelisted value (not the raw URL param) so invalid or malicious
values are never forwarded.
In `@assets/vue/components/subscribers/SubscriberModal.vue`:
- Around line 158-162: The watcher only observes props.isOpen so when
props.subscriberId changes while the modal is open the form isn't refreshed;
update the watcher to observe both props.isOpen and props.subscriberId (or use a
computed tuple like () => [props.isOpen, props.subscriberId]) and call
fetchSubscriberDetails() when the modal is open and subscriberId is present;
make sure the watcher callback logic still guards for newValue.open state and a
valid props.subscriberId before fetching to avoid unnecessary calls.
- Around line 12-14: Template dereferences subscriber.id while the component's
data property subscriber can be null and stale between loads; update the
template to safely access the id (e.g., use a null-safe expression like
subscriber?.id or a computed/displayId) and, in the component logic, explicitly
reset subscriber to null when starting a fetch and when closing the modal
(adjust the methods that load/close the modal—e.g., fetchSubscriber,
openModal/onOpen, closeModal/onClose—to set this.subscriber = null before/after
requests) so stale details are never rendered.
In `@src/Controller/AuthController.php`:
- Around line 75-79: The about() action in AuthController currently assumes
authClient->getSessionUser() always returns a valid user; update it to handle
null/failed responses and exceptions by checking the result of getSessionUser(),
returning a JsonResponse with 401 when no session/user is present, and catching
any exceptions from authClient->getSessionUser() to return a 503 with an error
payload; ensure you reference the about() method and
authClient->getSessionUser() and avoid throwing unhandled exceptions so the
endpoint always returns a controlled 401/503 response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af9f365d-eecc-4144-b87c-a3c04a012926
📒 Files selected for processing (7)
assets/vue/components/subscribers/SubscriberDirectory.vueassets/vue/components/subscribers/SubscriberModal.vuesrc/Controller/AuthController.phpsrc/Controller/SubscribersController.phpsrc/EventSubscriber/AuthGateSubscriber.phptests/System/ApplicationBundle/PhpListApplicationBundleTest.phptests/Unit/Controller/AuthControllerTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Unit/Controller/AuthControllerTest.php
- src/EventSubscriber/AuthGateSubscriber.php
- tests/System/ApplicationBundle/PhpListApplicationBundleTest.php
Comment on lines
68
to
+70
| $request->getSession()->remove('auth_token'); | ||
| $request->getSession()->remove('auth_id'); | ||
| $this->authClient->logout(); |
There was a problem hiding this comment.
Clear auth_expiry_date on logout too.
Line 49 stores auth_expiry_date, but Lines 68-70 only remove auth_token and auth_id. Leaving expiry metadata behind can produce stale-session behavior.
Suggested patch
public function logout(Request $request): Response
{
$request->getSession()->remove('auth_token');
+ $request->getSession()->remove('auth_expiry_date');
$request->getSession()->remove('auth_id');
$this->authClient->logout();
return $this->redirectToRoute('login');
}🤖 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 - 70, The logout sequence
in AuthController currently removes 'auth_token' and 'auth_id' but leaves
'auth_expiry_date' in the session, which can cause stale-session behavior;
update the logout logic (the block that calls $this->authClient->logout()) to
also remove 'auth_expiry_date' from the session (i.e., call
$request->getSession()->remove('auth_expiry_date') in the same place where
'auth_token' and 'auth_id' are removed).
| } | ||
|
|
||
| $afterId = (int) $request->query->get('after_id'); | ||
| $limit = max(1, (int) $request->query->get('limit', 10)); |
There was a problem hiding this comment.
Cap limit to prevent oversized fetch/export requests.
Line 40 and Line 104 accept caller-driven limits without an upper bound. A very high value can create expensive API calls and heavy responses.
Suggested patch
- $limit = max(1, (int) $request->query->get('limit', 10));
+ $limit = min(500, max(1, (int) $request->query->get('limit', 10)));
@@
- $collection = $this->subscribersClient->getSubscribers($filter, 0, $request->query->getInt('limit'));
+ $exportLimit = min(5000, max(1, $request->query->getInt('limit', 1000)));
+ $collection = $this->subscribersClient->getSubscribers($filter, 0, $exportLimit);Also applies to: 104-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Controller/SubscribersController.php` at line 40, The $limit pulled from
$request->query->get('limit', 10) must be capped to prevent oversized
fetch/export requests: introduce a MAX_LIMIT constant (e.g. private const
MAX_LIMIT = 1000) in SubscribersController and replace both instances where
$limit is computed ($limit = max(1, (int) $request->query->get('limit', 10)))
with a capped expression (use min(self::MAX_LIMIT, max(1, (int)
$request->query->get('limit', 10)))) so both the list/export code paths use the
same upper bound.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
assets/vue/components/subscribers/SubscriberModal.vue (1)
12-14:⚠️ Potential issue | 🔴 CriticalGuard nullable
subscriberreads and clear modal state on fetch/close.Line 13 can access
idwhilesubscriberisnull, and stale subscriber data is still retained across reopen because local state is not reset before fetch or on close.Suggested patch
- <h3 class="text-lg leading-6 font-medium text-slate-900" id="modal-title"> - Subscriber Details ID: {{ subscriber.id ?? '' }} - </h3> + <h3 class="text-lg leading-6 font-medium text-slate-900" id="modal-title"> + Subscriber Details ID: {{ subscriber?.id ?? props.subscriberId ?? '—' }} + </h3> @@ const fetchSubscriberDetails = async () => { + subscriber.value = null loading.value = true error.value = null @@ const close = () => { + subscriber.value = null + error.value = null emit('close') }Also applies to: 167-170, 202-204
🤖 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 12 - 14, The template and modal logic access subscriber properties when subscriber can be null and retain stale data across reopen; update all template bindings like "subscriber.id" to use safe access (subscriber?.id or v-if="subscriber") and ensure the component clears local subscriber state before starting a fetch and on close by resetting the data property (e.g., this.subscriber = null) inside the methods that open/fetch/close the modal (refer to the subscriber data prop and the methods fetchSubscriber and closeModal or whatever open/close handlers are present) so the modal shows no stale data while loading and is cleared when closed.assets/vue/components/subscribers/SubscriberDirectory.vue (1)
94-96:⚠️ Potential issue | 🟠 MajorClean up timer and active request on unmount.
Without teardown, a pending debounce/fetch can still resolve after unmount and mutate URL/state from a destroyed component context.
Suggested patch
-import {inject, onMounted, ref} from 'vue' +import {inject, onMounted, onUnmounted, ref} from 'vue' @@ let searchTimeout = null let fetchController = null + +onUnmounted(() => { + if (searchTimeout) { + clearTimeout(searchTimeout) + searchTimeout = null + } + fetchController?.abort() + fetchController = null +})Also applies to: 122-124, 186-227
🤖 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 94 - 96, The component starts debounce timers and async fetches (see the onMounted setup and the debounce/fetch logic around lines 122-124 and 186-227) but does not clear them on unmount; add an onUnmounted cleanup that clears any pending timeout (the ref holding the debounce timer) and cancels or ignores in-flight requests by using an AbortController for subscribersClient calls (or track an activeRequest token) and aborting it in onUnmounted, and ensure any callbacks check a "isUnmounted" flag or handle AbortError so state/URL updates in functions like the debounce handler and the fetchSubscribers logic are not run after unmount.
🤖 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/subscribers/SubscriberModal.vue`:
- Around line 15-17: The close button in SubscriberModal.vue is icon-only and
lacks an accessible name; update the button element that calls the close method
(the button with `@click`="close" and BaseIcon) to include an explicit accessible
label such as aria-label="Close" (or aria-label bound to a localized string)
and/or a visually-hidden label so screen readers announce the action; ensure the
BaseIcon remains unchanged and that the aria-label is present on the same button
element.
- Around line 167-185: fetchSubscriberDetails can run concurrently and allow an
older response to overwrite newer state; fix by adding a per-component request
token (e.g., latestRequestId as a ref/number) and incrementing it at start of
fetchSubscriberDetails, capture the currentRequestId in the closure, then only
assign subscriber.value and update formData.value (and set error.value) if
currentRequestId === latestRequestId; ensure loading.value is cleared
appropriately in finally but only mutate data when the token matches to prevent
stale overwrites of subscriber/form state from out-of-order responses
(references: fetchSubscriberDetails, subscribersClient.getSubscriber,
subscriber.value, formData.value, latestRequestId).
In `@src/Controller/SubscribersController.php`:
- Around line 53-57: The session array 'subscribers_history' in
SubscribersController grows unbounded because every new $afterId is appended; to
fix, cap the history to a fixed max size (e.g., MAX_HISTORY = 50) and implement
a sliding window when updating $history: after checking in_array($afterId,
$history, true) and appending $afterId, trim the array to the last N entries
(preserving order) before calling
$request->getSession()->set('subscribers_history', $history); update the logic
around $history, $afterId and the 'subscribers_history' session key to enforce
this limit.
---
Duplicate comments:
In `@assets/vue/components/subscribers/SubscriberDirectory.vue`:
- Around line 94-96: The component starts debounce timers and async fetches (see
the onMounted setup and the debounce/fetch logic around lines 122-124 and
186-227) but does not clear them on unmount; add an onUnmounted cleanup that
clears any pending timeout (the ref holding the debounce timer) and cancels or
ignores in-flight requests by using an AbortController for subscribersClient
calls (or track an activeRequest token) and aborting it in onUnmounted, and
ensure any callbacks check a "isUnmounted" flag or handle AbortError so
state/URL updates in functions like the debounce handler and the
fetchSubscribers logic are not run after unmount.
In `@assets/vue/components/subscribers/SubscriberModal.vue`:
- Around line 12-14: The template and modal logic access subscriber properties
when subscriber can be null and retain stale data across reopen; update all
template bindings like "subscriber.id" to use safe access (subscriber?.id or
v-if="subscriber") and ensure the component clears local subscriber state before
starting a fetch and on close by resetting the data property (e.g.,
this.subscriber = null) inside the methods that open/fetch/close the modal
(refer to the subscriber data prop and the methods fetchSubscriber and
closeModal or whatever open/close handlers are present) so the modal shows no
stale data while loading and is cleared when closed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb4a5c2a-890c-4d65-b438-89a843e82530
📒 Files selected for processing (4)
assets/vue/components/subscribers/SubscriberDirectory.vueassets/vue/components/subscribers/SubscriberModal.vuesrc/Controller/AuthController.phpsrc/Controller/SubscribersController.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Controller/AuthController.php
Comment on lines
+15
to
+17
| <button type="button" class="text-slate-400 hover:text-slate-500" @click="close"> | ||
| <BaseIcon name="close" class="w-5 h-5" /> | ||
| </button> |
There was a problem hiding this comment.
Add an accessible label to the icon-only close button.
Screen readers currently get no explicit action name for this control.
Suggested patch
- <button type="button" class="text-slate-400 hover:text-slate-500" `@click`="close">
+ <button
+ type="button"
+ aria-label="Close subscriber details modal"
+ class="text-slate-400 hover:text-slate-500"
+ `@click`="close"
+ >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button type="button" class="text-slate-400 hover:text-slate-500" @click="close"> | |
| <BaseIcon name="close" class="w-5 h-5" /> | |
| </button> | |
| <button | |
| type="button" | |
| aria-label="Close subscriber details modal" | |
| class="text-slate-400 hover:text-slate-500" | |
| `@click`="close" | |
| > | |
| <BaseIcon name="close" class="w-5 h-5" /> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/subscribers/SubscriberModal.vue` around lines 15 - 17,
The close button in SubscriberModal.vue is icon-only and lacks an accessible
name; update the button element that calls the close method (the button with
`@click`="close" and BaseIcon) to include an explicit accessible label such as
aria-label="Close" (or aria-label bound to a localized string) and/or a
visually-hidden label so screen readers announce the action; ensure the BaseIcon
remains unchanged and that the aria-label is present on the same button element.
Comment on lines
+167
to
+185
| const fetchSubscriberDetails = async () => { | ||
| loading.value = true | ||
| error.value = null | ||
| try { | ||
| subscriber.value = await subscribersClient.getSubscriber(props.subscriberId) | ||
|
|
||
| // Update formData | ||
| formData.value = { | ||
| email: subscriber.value.email, | ||
| confirmed: !!subscriber.value.confirmed, | ||
| blacklisted: !!subscriber.value.blacklisted, | ||
| htmlEmail: !!subscriber.value.htmlEmail, | ||
| disabled: !!subscriber.value.disabled | ||
| } | ||
| } catch (err) { | ||
| error.value = err.message | ||
| } finally { | ||
| loading.value = false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files assets/vue/components/subscribers/ | head -20Repository: phpList/web-frontend
Length of output: 446
🏁 Script executed:
wc -l assets/vue/components/subscribers/SubscriberModal.vueRepository: phpList/web-frontend
Length of output: 121
🏁 Script executed:
cat -n assets/vue/components/subscribers/SubscriberModal.vueRepository: phpList/web-frontend
Length of output: 9531
Add request deduplication to prevent stale data from overwriting current modal state.
The watch on subscriberId can trigger multiple concurrent fetchSubscriberDetails() calls. If an older request resolves after a newer one, it'll overwrite the current subscriber/form data with stale data. This happens naturally when users click between rows or if the modal reopens—the second request completes first, then the first request wipes it out.
Suggested patch
+let fetchSeq = 0
+
const fetchSubscriberDetails = async () => {
+ const seq = ++fetchSeq
subscriber.value = null
loading.value = true
error.value = null
try {
- subscriber.value = await subscribersClient.getSubscriber(props.subscriberId)
+ const data = await subscribersClient.getSubscriber(props.subscriberId)
+ if (seq !== fetchSeq) return
+ subscriber.value = data
@@
} catch (err) {
+ if (seq !== fetchSeq) return
error.value = err.message
} finally {
+ if (seq !== fetchSeq) return
loading.value = false
}
}🤖 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 167 -
185, fetchSubscriberDetails can run concurrently and allow an older response to
overwrite newer state; fix by adding a per-component request token (e.g.,
latestRequestId as a ref/number) and incrementing it at start of
fetchSubscriberDetails, capture the currentRequestId in the closure, then only
assign subscriber.value and update formData.value (and set error.value) if
currentRequestId === latestRequestId; ensure loading.value is cleared
appropriately in finally but only mutate data when the token matches to prevent
stale overwrites of subscriber/form state from out-of-order responses
(references: fetchSubscriberDetails, subscribersClient.getSubscriber,
subscriber.value, formData.value, latestRequestId).
Comment on lines
+53
to
+57
| $history = $request->getSession()->get('subscribers_history', []); | ||
| if (!in_array($afterId, $history, true)) { | ||
| $history[] = $afterId; | ||
| $request->getSession()->set('subscribers_history', $history); | ||
| } |
There was a problem hiding this comment.
Session history can grow unbounded.
Every new afterId appends to subscribers_history without any pruning. During long browsing sessions with lots of pagination, this array keeps growing. Consider capping the history size or using a sliding window.
$history = $request->getSession()->get('subscribers_history', []);
if (!in_array($afterId, $history, true)) {
$history[] = $afterId;
+ // Keep only the last N 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 - 57, The session
array 'subscribers_history' in SubscribersController grows unbounded because
every new $afterId is appended; to fix, cap the history to a fixed max size
(e.g., MAX_HISTORY = 50) and implement a sliding window when updating $history:
after checking in_array($afterId, $history, true) and appending $afterId, trim
the array to the last N entries (preserving order) before calling
$request->getSession()->set('subscribers_history', $history); update the logic
around $history, $afterId and the 'subscribers_history' session key to enforce
this limit.
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
Styling & UI
Improvements
Thanks for contributing to phpList!