Subscribe page#87
Open
TatevikGr wants to merge 63 commits into
Open
Conversation
…page # Conflicts: # composer.json
…ger and streamline attribute configuration
… improved clarity and efficiency
…efactoring form rendering logic
…mage retrieval routes
|
Warning Review limit reached
More reviews will be available in 5 minutes and 42 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR introduces a comprehensive public subscribe pages feature that allows administrators to create customizable subscription forms with support for multi-step configuration (title, mailing lists, subscriber attributes, transactional messages, and email settings). The implementation includes a public-facing subscribe/unsubscribe interface with form validation, language localization, and admin controls. Additionally, the PR refactors authentication redirects across the application to safely preserve user navigation intent via URL parameters, standardizes UI styling through consistent checkbox accent colors and button borders, and delivers extensive test coverage including component and integration tests. Sequence Diagram(s)sequenceDiagram
participant Browser
participant PublicController as PublicSubscribeController
participant SubscribeAPI as SubscribePagesClient
participant RestAPI as Subscriber API
Browser->>PublicController: GET /subscribe/{pageId}
PublicController->>SubscribeAPI: getSubscribePage(pageId)
SubscribeAPI-->>PublicController: SubscribePage data
PublicController->>Browser: render subscribe form (language_texts, attributes, lists)
Browser->>PublicController: POST subscribe form
PublicController->>SubscribeAPI: create subscription (PublicSubscriptionRequest) per list
SubscribeAPI->>RestAPI: POST subscriptions
RestAPI-->>SubscribeAPI: 200/422 errors
SubscribeAPI-->>PublicController: result / errors
PublicController->>Browser: render success or form with errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major 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 (2)
src/Security/SessionAuthenticator.php (1)
54-57: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExtract duplicated helper methods to a shared service or trait.
Four methods are duplicated between
SessionAuthenticatorandAuthGateSubscriber:
normalizePathbuildLoginUrlisSafeRedirectTargetmatchesPrefixThis duplication creates maintenance burden and risk of divergence.
♻️ Suggested approach
Create a shared service or trait:
// src/Security/RedirectHelper.php class RedirectHelper { public function __construct(private readonly UrlGeneratorInterface $urlGenerator) { } public function buildLoginUrl(string $redirectTarget): string { $loginUrl = $this->urlGenerator->generate('login'); if (!$this->isSafeRedirectTarget($redirectTarget)) { return $loginUrl; } return $loginUrl . '?' . http_build_query(['redirect' => $redirectTarget]); } public function isSafeRedirectTarget(string $target): bool { if (!str_starts_with($target, '/') || str_starts_with($target, '//')) { return false; } $path = parse_url($target, PHP_URL_PATH); if (!is_string($path)) { return false; } $normalizedPath = $this->normalizePath($path); return $normalizedPath !== '/login' && !str_starts_with($normalizedPath, '/login'); } public function normalizePath(string $path): string { return (string) preg_replace('#^/(?:app|app_test)\.php#', '', $path, 1); } public function matchesPrefix(string $path, string $prefix): bool { if (str_ends_with($prefix, '/')) { $exactPath = rtrim($prefix, '/'); return $path === $exactPath || str_starts_with($path, $prefix); } return str_starts_with($path, $prefix); } }Then inject and use in both classes.
Also applies to: 94-103, 105-119, 121-129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Security/SessionAuthenticator.php` around lines 54 - 57, Extract the four duplicated methods (normalizePath, buildLoginUrl, isSafeRedirectTarget, matchesPrefix) into a single shared helper (either a service class or a trait) and inject/compose it into SessionAuthenticator and AuthGateSubscriber; implement the helper to accept UrlGeneratorInterface in its constructor (or require it when used), move the logic shown in the suggestion into that helper, replace the local method definitions in SessionAuthenticator and AuthGateSubscriber with calls to the helper (e.g., $this->redirectHelper->buildLoginUrl(...)) and update constructor signatures to receive the helper via dependency injection; ensure all calls to normalizePath, buildLoginUrl, isSafeRedirectTarget and matchesPrefix in both classes are redirected to the shared helper and remove the duplicated method implementations.templates/public/base.html.twig (1)
3-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix document structure: body content is currently rendered inside
<head>.
</head>,<body>, and closing tags are missing, so page markup is invalid and can render inconsistently.Suggested patch
<html lang="en"> <head> @@ {% block javascripts %} <script src="{{ asset('build/runtime.js', 'phplist_web_frontend') }}" defer></script> <script src="{{ asset('build/app.js', 'phplist_web_frontend') }}" defer></script> {% endblock %} +</head> +<body> {{ data['header']|unescape_header|raw }} {% block body %}{% endblock %} {{ signature|raw }} {{ data['footer']|unescape_header|raw }} +</body> +</html>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@templates/public/base.html.twig` around lines 3 - 24, The head currently contains body-level content; fix the HTML structure by closing the head and opening a body element so that block javascripts remains in <head> but data['header']|unescape_header, the {% block body %}{% endblock %}, {{ signature }} and data['footer']|unescape_header are moved outside the head into the <body>, and ensure the <body> is properly closed along with the document; update the template around the block javascripts, data['header']|unescape_header, {% block body %}, signature, and data['footer']|unescape_header to reflect this corrected head/body separation.
🟡 Minor comments (7)
assets/styles/subscribe.css-13-15 (1)
13-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the dedicated header background token here.
#headeris currently tied to--primary-text, which makes header styling depend on text color instead of the header token defined for this surface.💡 Suggested patch
`#header` { - background: var(--primary-text); + background: var(--header-bg); box-sizing: border-box; position: relative;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/styles/subscribe.css` around lines 13 - 15, Replace the header's background variable so `#header` uses the dedicated header background token instead of the text color token: change the background assignment in the `#header` rule from var(--primary-text) to the header background token (for example var(--header-background) or your project's defined token name) so the header styling relies on the correct surface token rather than --primary-text.assets/vue/components/public-pages/PublicPagesDirectory.vue-59-59 (1)
59-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winActions cell colspan is misaligned with table structure.
Line 59 sets
colspan="5"for the actions cell in a row that already has 5 preceding cells and 6 total headers, which can break alignment/semantics.Proposed fix
- <td colspan="5" class="px-6 py-3"> + <td class="px-6 py-3">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/vue/components/public-pages/PublicPagesDirectory.vue` at line 59, The actions cell uses colspan="5" which misaligns the table (there are already 5 preceding cells and 6 total headers); update the <td> for the actions cell in PublicPagesDirectory.vue (the template row that renders the actions cell) to use colspan="1" or remove the colspan attribute so the actions column aligns with the sixth header, keeping the table semantics intact.src/Command/PublishPhplistTextsCommand.php-54-57 (1)
54-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
glob()result before iterating.On Line 54,
glob()may returnfalse; iterating that value can emit warnings during install/update hooks. Coalesce to an array beforeforeach.Suggested patch
- foreach (glob($sourceDir . '/*.inc') as $file) { + $files = glob($sourceDir . '/*.inc') ?: []; + foreach ($files as $file) { $filename = basename($file); $this->filesystem->copy($file, $targetDir . '/' . $filename, true); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Command/PublishPhplistTextsCommand.php` around lines 54 - 57, The foreach iterates glob($sourceDir . '/*.inc') which can return false; change to guard/coalesce the result (e.g. assign to a variable like $files = glob($sourceDir . '/*.inc') ?: []; ) and iterate $files so no warning is emitted; update the block in PublishPhplistTextsCommand where foreach(...) currently copies files using $this->filesystem->copy($file, $targetDir . '/' . $filename, true) to use the guarded $files array.src/Service/FormDataMapper.php-177-183 (1)
177-183:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse value-based parsing for
htmlemailin the default branch.Line 182 currently treats any present
htmlemailkey as opt-in. A submitted value like'0'is still interpreted astrue.Suggested patch
private function resolveHtmlEmailFromRequest(string $htmlChoice, array $posted): bool { return match ($htmlChoice) { 'textonly' => false, 'htmlonly' => true, 'checkfortext' => !isset($posted['textemail']), 'radiotext', 'radiohtml' => ((int) ($posted['htmlemail'] ?? 0)) === 1, - default => isset($posted['htmlemail']), + default => $this->isTruthy($posted['htmlemail'] ?? null), }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Service/FormDataMapper.php` around lines 177 - 183, The default branch in the match on $htmlChoice in FormDataMapper.php treats any presence of the 'htmlemail' key as true, so values like '0' are incorrectly opt-in; change the default branch to parse the posted['htmlemail'] value rather than its existence (e.g. use a boolean/value-based parse such as casting to int or using FILTER_VALIDATE_BOOLEAN on $posted['htmlemail'] with a sensible default) so only truthy submissions count as true; update the return branch that currently uses isset($posted['htmlemail']) to use this value-parsing approach referencing $posted['htmlemail'] and the surrounding match on $htmlChoice.src/Service/LanguageService.php-40-42 (1)
40-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the redundant identity function.
The
array_mapwith an identity function doesn't transform the array in any way—it's equivalent to just returningget_defined_vars()directly. Additionally,get_defined_vars()will include the$pathparameter itself in the returned array, which probably isn't intended as a "language text."♻️ Simplify by removing array_map
$loader = static function (string $path): array { require $path; - - return array_map(function ($value) { - return $value; - }, get_defined_vars()); + return get_defined_vars(); };If you need to exclude
$pathfrom the result, you can unset it explicitly:$loader = static function (string $path): array { require $path; $vars = get_defined_vars(); unset($vars['path']); return $vars; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Service/LanguageService.php` around lines 40 - 42, The anonymous loader closure currently returns get_defined_vars() wrapped in an identity array_map; replace that with direct handling: after require $path, capture $vars = get_defined_vars(), unset($vars['path']) to remove the injected $path parameter, and return $vars; update the closure referenced as $loader (or the static function (string $path): array) to remove the redundant array_map and explicitly exclude $path from the returned array.src/Controller/PublicSubscribeController.php-51-53 (1)
51-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn 400 Bad Request instead of 404 for invalid email format.
Throwing
createNotFoundException()returns a 404 Not Found status, but an invalid email format is a client error that should return 400 Bad Request. A 404 suggests the resource (unsubscribe page) doesn't exist, not that the input is malformed.🔧 Use BadRequestHttpException for input validation
+use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; + ... if (!filter_var($email, FILTER_VALIDATE_EMAIL)) { - throw $this->createNotFoundException('Invalid email address.'); + throw new BadRequestHttpException('Invalid email address.'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Controller/PublicSubscribeController.php` around lines 51 - 53, The controller currently throws createNotFoundException() when filter_var($email, FILTER_VALIDATE_EMAIL) fails, causing a 404; change this to throw a BadRequestHttpException with the same message so the response is 400 Bad Request. Locate the email validation in PublicSubscribeController (the method that reads $email and calls filter_var) and replace the createNotFoundException usage with throwing a BadRequestHttpException('Invalid email address.') so clients receive a proper 400 for malformed input.src/Service/PublicSubscribeFormBuilder.php-35-39 (1)
35-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAttribute comparator is currently reversed and sorts in the opposite direction.
At Line 37, callback params are inverted (
$right, $left) while the comparison is$left <=> $right, which effectively produces descending order and can render attributes out of expectedlist_order.Suggested fix
usort( $builtAttributes, - static fn (array $right, array $left): int => + static fn (array $left, array $right): int => [$left['type'], $left['list_order']] <=> [$right['type'], $right['list_order']] );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Service/PublicSubscribeFormBuilder.php` around lines 35 - 39, The usort comparator for $builtAttributes in PublicSubscribeFormBuilder is inverted: the callback arguments are declared as (array $right, array $left) but compared as [$left['type'], $left['list_order']] <=> [$right['type'], $right['list_order']], producing reverse ordering; fix by making the comparator consistent—either change the lambda signature to (array $left, array $right): int => [$left['type'], $left['list_order']] <=> [$right['type'], $right['list_order']] or keep the current parameters and swap the operands to [$right['type'], $right['list_order']] <=> [$left['type'], $left['list_order']] so that sorting by 'type' then 'list_order' yields the intended ascending order.
🧹 Nitpick comments (5)
src/EventSubscriber/UnauthorizedSubscriber.php (1)
76-90: ⚡ Quick winConsider extracting shared redirect validation logic.
The
isSafeRedirectTarget()method is duplicated verbatim inAuthController. If the validation rules ever need updating, both locations must stay in sync. A shared trait or service could centralize this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/EventSubscriber/UnauthorizedSubscriber.php` around lines 76 - 90, The isSafeRedirectTarget(string $target) logic is duplicated in UnauthorizedSubscriber and AuthController; extract this validation into a single shared place (either a RedirectValidator service class or a RedirectTrait) and have both UnauthorizedSubscriber::isSafeRedirectTarget and AuthController call the shared implementation; move the existing logic (including parse_url, preg_replace normalization, and checks for leading '//' and '/login') into the new RedirectValidator::isSafeRedirectTarget or RedirectTrait::isSafeRedirectTarget, inject or use that in UnauthorizedSubscriber and AuthController, and remove the duplicate method from one of them so updates are centralized.tests/Unit/EventSubscriber/AuthGateSubscriberTest.php (1)
1-116: ⚖️ Poor tradeoffConsider adding edge case tests for redirect validation.
The current tests cover basic scenarios well. For additional confidence in the security validation logic, consider adding tests for:
- Path traversal attempts:
/../../admin- Encoded characters:
/%2F%2Fevil.com- Login redirect blocking:
/login?foo=bar- Malformed URLs
These tests would verify defense-in-depth behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/EventSubscriber/AuthGateSubscriberTest.php` around lines 1 - 116, Add unit tests in AuthGateSubscriberTest that exercise onKernelRequest with malicious or malformed paths to ensure redirects are validated: create Request instances for "/../../admin", "/%2F%2Fevil.com", "/login?foo=bar", and an obviously malformed URL, attach a Session mock without 'auth_token', wrap each in a RequestEvent and call AuthGateSubscriber::onKernelRequest, then assert the outcome (either no external redirect and/or a safe RedirectResponse to the internal login path with sanitized or omitted redirect param). Use Request, RequestEvent, RedirectResponse and the existing $this->subscriber to locate the logic under test and mirror the existing test patterns.src/EventSubscriber/AuthGateSubscriber.php (1)
106-120: ⚖️ Poor tradeoffConsider normalizing path traversal sequences for defense-in-depth.
The validation correctly blocks protocol-relative URLs and login redirects, but doesn't normalize path traversal sequences like
/../or/./. While the redirect stays within the application (limiting exploit potential), adding path normalization would strengthen security.Consider using
realpath()or manually collapsing..and.segments after validation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/EventSubscriber/AuthGateSubscriber.php` around lines 106 - 120, The isSafeRedirectTarget function needs path-segment normalization to collapse "." and ".." sequences before checking against "/login"; update normalizePath (or add a new helper) to split the parsed path from parse_url, resolve and remove "." segments and properly handle ".." segments without using realpath (so it won't depend on the filesystem) — ensure the resolved path always begins with "/" and never escapes above the application root (e.g., reduce "/a/b/../c" to "/a/c", turn "/../" into "/") — then use that normalized value in isSafeRedirectTarget's comparison and str_starts_with checks.assets/vue/components/campaigns/CampaignDirectory.spec.js (1)
151-154: ⚡ Quick winMake
clickFilterfail with a clear message when a filter button is missing.Right now a missing label crashes with a generic
undefined.triggererror, which makes failures noisier than needed.💡 Suggested patch
const clickFilter = async (wrapper, label) => { const btn = wrapper.findAll('button[type="button"]').find((b) => b.text().trim() === label) + if (!btn) { + throw new Error(`Filter button "${label}" not found`) + } await btn.trigger('click') await flushPromises() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/vue/components/campaigns/CampaignDirectory.spec.js` around lines 151 - 154, The helper clickFilter currently assumes a button exists and will raise a generic undefined.trigger error when missing; update the clickFilter function to check the found button (the btn variable from wrapper.findAll('button[type="button"]').find(...)) and if it's falsy throw a clear Error (e.g. `Filter button "${label}" not found`) before attempting await btn.trigger('click') and await flushPromises(), so test failures report the missing label explicitly.src/Service/LanguageService.php (1)
35-39: ⚡ Quick winSwap the parameter names to match the comparison order.
The callback parameters are named
$right, $leftbut the comparison reads$left <=> $right, which is backwards from the typical convention and inconsistent with the options sort at lines 47–51 that uses$left, $rightin the correct order.🔄 Standardize parameter order
usort( $builtAttributes, - static fn (array $right, array $left): int => + static fn (array $left, array $right): int => [$left['type'], $left['list_order']] <=> [$right['type'], $right['list_order']] );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Service/LanguageService.php` around lines 35 - 39, The comparator callback currently declares its parameters in the wrong order (named $right, $left) while using the expression $left <=> $right and elsewhere (the options sort at lines 47–51) uses $left, $right; rename the comparator parameters to $left and $right (keeping the same comparator logic) so the parameter order matches the comparison expression and the options sort convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets/vue/api.js`:
- Around line 32-34: The redirect construction currently includes
window.location.hash (built into redirectTarget) which can leak fragment data to
the server; update the code that builds redirectTarget used with
AUTHENTICATION_REDIRECT_PATH to exclude window.location.hash (use only
window.location.pathname + window.location.search or their sanitized
equivalents) and ensure the URLSearchParams call still encodes the resulting
redirect value appropriately before assigning window.location.href.
In `@assets/vue/components/campaigns/ViewCampaignModal.vue`:
- Around line 95-96: The data-testid is placed on the checkbox input (the input
element for "send to lists" option) but tests expect it on the actual "Send to
lists" button; remove data-testid="send-btn" from the checkbox input and add
data-testid="send-btn" to the button element that renders the "Send to lists"
action (the button with the label/text "Send to lists" in ViewCampaignModal.vue)
so the tests check the button's disabled state instead of the checkbox.
In `@assets/vue/components/public-pages/PublicPageEditor.vue`:
- Line 634: The bug is that form.value.displayListCategories is a string
('0'/'1') so a truthy check always yields 'yes'; update the serialization to
test the actual value (e.g. compare against '1' or coerce to number/boolean) so
['showcategories', form.value.displayListCategories === '1' ? 'yes' : 'no'] (or
equivalent) is used wherever showcategories is constructed to ensure '0' becomes
'no' and '1' becomes 'yes'.
- Around line 551-554: applyLoadedDataToForm() assumes a page object and
dereferences page.data when iterating form.value.attributes (the block using
form.value.attributes.split(',').forEach and looking up page.data), which causes
a null dereference in the create flow; fix by guarding against a missing page
before accessing page.data (e.g., return or skip attribute processing when page
is falsy or use optional chaining to only lookup attributes if page?.data
exists) so the loop does not attempt to read page.data when no page is supplied.
In `@assets/vue/components/public-pages/PublicPagesDirectory.vue`:
- Line 36: The template currently accesses page.owner.loginName directly (e.g.,
in the table cell and the other occurrence around the same component), which can
throw if page.owner is null/undefined; update those bindings to safely handle
missing owner by using optional chaining or a conditional fallback (e.g.,
page.owner?.loginName || 'Unassigned' or a localized empty placeholder) wherever
page.owner.loginName is referenced in PublicPagesDirectory.vue (both the
occurrence shown and the similar use at the later occurrence), ensuring the UI
renders a sensible fallback instead of crashing.
- Around line 324-327: handleSetDefault is currently a no-op; implement
persistence by performing the API request to update
phplist_config.defaultsubscribepage with the selected page id inside the
existing withRowTask wrapper. In the async callback for
handleSetDefault(targetPage) call the appropriate API client or fetch/axios to
PUT/POST { defaultsubscribepage: targetPage.id } to the phplist_config endpoint,
handle errors and show success/failure UI feedback, and then update local state
(or reload pages) so the UI reflects the new default; keep the
withRowTask(targetPage.id, ...) wrapper and use the same function name
handleSetDefault to locate the change.
In `@config/packages/security.yaml`:
- Around line 24-25: The current security.yaml path regexes for the public
routes "subscribe" and "unsubscribe" are unanchored and match any path that
starts with those segments, which risks accidental auth bypass for prefixed
routes; update the two path patterns (the lines containing
^/(?:app(?:_test)?\.php/)?subscribe and ^/(?:app(?:_test)?\.php/)?unsubscribe)
to require the segment boundary after the name (e.g., append a non-capturing
group like (?:$|/) or a trailing $ when appropriate) so they only match the
exact segment or its immediate subpaths, then keep roles: PUBLIC_ACCESS
unchanged.
In `@openapi.json`:
- Around line 5215-5239: The DELETE operation under the "subscribe-pages"
endpoint (operationId "48751c56d39746c6b8dc3f782a9b22e5") currently exposes
subscriber PII by accepting "email" as a query parameter; change the spec to
remove the "email" query parameter and instead accept either an unsubscribe
request body (e.g., JSON with "email" and optional "listId") or a single opaque
"unsubscribeToken" in the request body, and add an explicit "listId" field if
unsubscribe is list-specific; update the operation's parameters/schema
accordingly and mirror the same change for the corresponding other operation
(the similar block at lines 8752-8772) so the API no longer leaks email in URLs
and supports unambiguous per-list unsubscribe.
- Around line 1396-1407: The OpenAPI change replaced the original
/api/v2/bounces 200 response (which returned BounceView[]) with an object {
items, pagination }, breaking v2 clients; revert the v2 response to the original
array shape (return BounceView[] for the GET /api/v2/bounces response schema)
and add a new contract for the paginated variant (e.g. introduce a new component
schema PaginatedBounceList with properties items: BounceView[] and pagination:
CursorPagination and expose it via a new path or versioned operation like
/api/v3/bounces or /api/v2/bounces/paginated); update the paths section for the
GET operation that currently references the new object to point back to the
array schema and add the new paginated schema/path instead.
- Around line 4907-4914: The OpenAPI spec made the "php-auth-pw" header optional
for the admin GET endpoint /api/v2/subscribe-pages/{id}; change the header
object's "required" from false to true so the header is required for that
operation (ensure the schema block for "php-auth-pw" under the
/api/v2/subscribe-pages/{id} operation is set to required: true), keeping admin
reads authenticated while public reads remain under
/api/v2/public/subscribe-pages/{pageId}.
In `@src/Controller/PublicSubscribeController.php`:
- Around line 68-73: The getStylesheets (and the analogous image-serving action)
currently concatenates the raw {fileName} into a filesystem path allowing path
traversal; fix by enforcing a strict route requirement regex on the {fileName}
placeholder (e.g. only allow alphanumerics, dots, underscores, hyphens) and
sanitize the incoming parameter with basename() before building the path in
PublicSubscribeController::getStylesheets (and the images method), then compute
the path relative to the known public/build directory and only serve the file if
it exists and is inside that directory.
In `@src/Service/ListSelectionService.php`:
- Around line 29-37: The validation messages in ListSelectionService.php are
hardcoded English strings; update the method that uses $selectedLists and
$availableListIds (the block creating $allowedListLookup and returning error
strings) to return translatable message keys (e.g. 'newsletter.select_required'
and 'newsletter.select_invalid') or use the app's translator service instead of
literal English text; ensure the controller/template performs the translation so
public/localized flows show localized messages instead of mixed-language
hardcoded strings.
In `@src/Service/PhpListTranslationLoader.php`:
- Around line 68-76: The locale lookup in mapLocaleToPhpListFile currently only
matches exact keys ('en','es','fr') so regional variants like 'fr_FR' or 'es-MX'
fall back to English; normalize the incoming $locale (e.g., lowercase and strip
region tags by splitting on '_' or '-' and using the primary language subtag or
substr($locale,0,2)) before checking the $map array so regional/localized codes
map correctly to the intended files in mapLocaleToPhpListFile.
In `@src/Service/PublicSubscribeFormValidator.php`:
- Line 34: The current normalization of $formData['selected_lists'] in
PublicSubscribeFormValidator.php uses array_map directly which will throw a
TypeError if the input is scalar; fix by first checking
is_array($formData['selected_lists']) and only then run array_map('intval',
...), array_unique and array_values to populate $selectedLists; if the check
fails (not set or not an array) set $selectedLists = [] (or an appropriate
default) so malformed input on the public route does not cause a server error.
In `@tests/Integration/Controller/PublicSubscribeControllerPantherTest.php`:
- Around line 79-82: The selectors in the assertions are brittle—replace the
legacy class-based selectors used in assertSelectorExists ('form.legacy-form',
'input#email[name="email"][type="email"]',
'button[type="submit"].legacy-button') with behavior-focused selectors: look for
a form element (e.g. 'form' or 'form[action]' or a form with role), the email
input by attribute only (e.g. 'input[type="email"][name="email"]' without
relying on id/class), and the submit control by type (e.g.
'button[type="submit"], input[type="submit"]'); update the three
assertSelectorExists calls accordingly in the test so they no longer depend on
legacy styling hooks.
---
Outside diff comments:
In `@src/Security/SessionAuthenticator.php`:
- Around line 54-57: Extract the four duplicated methods (normalizePath,
buildLoginUrl, isSafeRedirectTarget, matchesPrefix) into a single shared helper
(either a service class or a trait) and inject/compose it into
SessionAuthenticator and AuthGateSubscriber; implement the helper to accept
UrlGeneratorInterface in its constructor (or require it when used), move the
logic shown in the suggestion into that helper, replace the local method
definitions in SessionAuthenticator and AuthGateSubscriber with calls to the
helper (e.g., $this->redirectHelper->buildLoginUrl(...)) and update constructor
signatures to receive the helper via dependency injection; ensure all calls to
normalizePath, buildLoginUrl, isSafeRedirectTarget and matchesPrefix in both
classes are redirected to the shared helper and remove the duplicated method
implementations.
In `@templates/public/base.html.twig`:
- Around line 3-24: The head currently contains body-level content; fix the HTML
structure by closing the head and opening a body element so that block
javascripts remains in <head> but data['header']|unescape_header, the {% block
body %}{% endblock %}, {{ signature }} and data['footer']|unescape_header are
moved outside the head into the <body>, and ensure the <body> is properly closed
along with the document; update the template around the block javascripts,
data['header']|unescape_header, {% block body %}, signature, and
data['footer']|unescape_header to reflect this corrected head/body separation.
---
Minor comments:
In `@assets/styles/subscribe.css`:
- Around line 13-15: Replace the header's background variable so `#header` uses
the dedicated header background token instead of the text color token: change
the background assignment in the `#header` rule from var(--primary-text) to the
header background token (for example var(--header-background) or your project's
defined token name) so the header styling relies on the correct surface token
rather than --primary-text.
In `@assets/vue/components/public-pages/PublicPagesDirectory.vue`:
- Line 59: The actions cell uses colspan="5" which misaligns the table (there
are already 5 preceding cells and 6 total headers); update the <td> for the
actions cell in PublicPagesDirectory.vue (the template row that renders the
actions cell) to use colspan="1" or remove the colspan attribute so the actions
column aligns with the sixth header, keeping the table semantics intact.
In `@src/Command/PublishPhplistTextsCommand.php`:
- Around line 54-57: The foreach iterates glob($sourceDir . '/*.inc') which can
return false; change to guard/coalesce the result (e.g. assign to a variable
like $files = glob($sourceDir . '/*.inc') ?: []; ) and iterate $files so no
warning is emitted; update the block in PublishPhplistTextsCommand where
foreach(...) currently copies files using $this->filesystem->copy($file,
$targetDir . '/' . $filename, true) to use the guarded $files array.
In `@src/Controller/PublicSubscribeController.php`:
- Around line 51-53: The controller currently throws createNotFoundException()
when filter_var($email, FILTER_VALIDATE_EMAIL) fails, causing a 404; change this
to throw a BadRequestHttpException with the same message so the response is 400
Bad Request. Locate the email validation in PublicSubscribeController (the
method that reads $email and calls filter_var) and replace the
createNotFoundException usage with throwing a BadRequestHttpException('Invalid
email address.') so clients receive a proper 400 for malformed input.
In `@src/Service/FormDataMapper.php`:
- Around line 177-183: The default branch in the match on $htmlChoice in
FormDataMapper.php treats any presence of the 'htmlemail' key as true, so values
like '0' are incorrectly opt-in; change the default branch to parse the
posted['htmlemail'] value rather than its existence (e.g. use a
boolean/value-based parse such as casting to int or using
FILTER_VALIDATE_BOOLEAN on $posted['htmlemail'] with a sensible default) so only
truthy submissions count as true; update the return branch that currently uses
isset($posted['htmlemail']) to use this value-parsing approach referencing
$posted['htmlemail'] and the surrounding match on $htmlChoice.
In `@src/Service/LanguageService.php`:
- Around line 40-42: The anonymous loader closure currently returns
get_defined_vars() wrapped in an identity array_map; replace that with direct
handling: after require $path, capture $vars = get_defined_vars(),
unset($vars['path']) to remove the injected $path parameter, and return $vars;
update the closure referenced as $loader (or the static function (string $path):
array) to remove the redundant array_map and explicitly exclude $path from the
returned array.
In `@src/Service/PublicSubscribeFormBuilder.php`:
- Around line 35-39: The usort comparator for $builtAttributes in
PublicSubscribeFormBuilder is inverted: the callback arguments are declared as
(array $right, array $left) but compared as [$left['type'], $left['list_order']]
<=> [$right['type'], $right['list_order']], producing reverse ordering; fix by
making the comparator consistent—either change the lambda signature to (array
$left, array $right): int => [$left['type'], $left['list_order']] <=>
[$right['type'], $right['list_order']] or keep the current parameters and swap
the operands to [$right['type'], $right['list_order']] <=> [$left['type'],
$left['list_order']] so that sorting by 'type' then 'list_order' yields the
intended ascending order.
---
Nitpick comments:
In `@assets/vue/components/campaigns/CampaignDirectory.spec.js`:
- Around line 151-154: The helper clickFilter currently assumes a button exists
and will raise a generic undefined.trigger error when missing; update the
clickFilter function to check the found button (the btn variable from
wrapper.findAll('button[type="button"]').find(...)) and if it's falsy throw a
clear Error (e.g. `Filter button "${label}" not found`) before attempting await
btn.trigger('click') and await flushPromises(), so test failures report the
missing label explicitly.
In `@src/EventSubscriber/AuthGateSubscriber.php`:
- Around line 106-120: The isSafeRedirectTarget function needs path-segment
normalization to collapse "." and ".." sequences before checking against
"/login"; update normalizePath (or add a new helper) to split the parsed path
from parse_url, resolve and remove "." segments and properly handle ".."
segments without using realpath (so it won't depend on the filesystem) — ensure
the resolved path always begins with "/" and never escapes above the application
root (e.g., reduce "/a/b/../c" to "/a/c", turn "/../" into "/") — then use that
normalized value in isSafeRedirectTarget's comparison and str_starts_with
checks.
In `@src/EventSubscriber/UnauthorizedSubscriber.php`:
- Around line 76-90: The isSafeRedirectTarget(string $target) logic is
duplicated in UnauthorizedSubscriber and AuthController; extract this validation
into a single shared place (either a RedirectValidator service class or a
RedirectTrait) and have both UnauthorizedSubscriber::isSafeRedirectTarget and
AuthController call the shared implementation; move the existing logic
(including parse_url, preg_replace normalization, and checks for leading '//'
and '/login') into the new RedirectValidator::isSafeRedirectTarget or
RedirectTrait::isSafeRedirectTarget, inject or use that in
UnauthorizedSubscriber and AuthController, and remove the duplicate method from
one of them so updates are centralized.
In `@src/Service/LanguageService.php`:
- Around line 35-39: The comparator callback currently declares its parameters
in the wrong order (named $right, $left) while using the expression $left <=>
$right and elsewhere (the options sort at lines 47–51) uses $left, $right;
rename the comparator parameters to $left and $right (keeping the same
comparator logic) so the parameter order matches the comparison expression and
the options sort convention.
In `@tests/Unit/EventSubscriber/AuthGateSubscriberTest.php`:
- Around line 1-116: Add unit tests in AuthGateSubscriberTest that exercise
onKernelRequest with malicious or malformed paths to ensure redirects are
validated: create Request instances for "/../../admin", "/%2F%2Fevil.com",
"/login?foo=bar", and an obviously malformed URL, attach a Session mock without
'auth_token', wrap each in a RequestEvent and call
AuthGateSubscriber::onKernelRequest, then assert the outcome (either no external
redirect and/or a safe RedirectResponse to the internal login path with
sanitized or omitted redirect param). Use Request, RequestEvent,
RedirectResponse and the existing $this->subscriber to locate the logic under
test and mirror the existing test patterns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aea7f57c-bf3b-4a56-a378-2ac5b0266bca
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (75)
.github/workflows/ci.ymlassets/router/index.jsassets/styles/color.cssassets/styles/subscribe.cssassets/vue/api.jsassets/vue/components/base/BaseBadge.spec.jsassets/vue/components/base/BaseCard.spec.jsassets/vue/components/base/BaseCard.vueassets/vue/components/bounces/BounceOverview.spec.jsassets/vue/components/bounces/BounceOverview.vueassets/vue/components/bounces/BouncePer.spec.jsassets/vue/components/bounces/BounceRules.spec.jsassets/vue/components/bounces/BounceRules.vueassets/vue/components/bounces/BouncesActionsPanel.spec.jsassets/vue/components/bounces/BouncesActionsPanel.vueassets/vue/components/campaigns/CampaignDirectory.spec.jsassets/vue/components/campaigns/CampaignDirectory.vueassets/vue/components/campaigns/ViewCampaignModal.spec.jsassets/vue/components/campaigns/ViewCampaignModal.vueassets/vue/components/lists/AddSubscribersModal.vueassets/vue/components/lists/CreateListModal.vueassets/vue/components/lists/EditListModal.vueassets/vue/components/lists/ListDirectory.vueassets/vue/components/lists/ListSubscribersExportPanel.vueassets/vue/components/public-pages/PublicPageEditor.spec.jsassets/vue/components/public-pages/PublicPageEditor.vueassets/vue/components/public-pages/PublicPagesDirectory.spec.jsassets/vue/components/public-pages/PublicPagesDirectory.vueassets/vue/components/subscribers/SubscriberDirectory.vueassets/vue/components/subscribers/SubscriberModal.vueassets/vue/components/subscribers/SubscriberTable.vueassets/vue/components/templates/TemplateLibrary.vueassets/vue/views/CampaignEditView.vueassets/vue/views/ListSubscribersView.vueassets/vue/views/PublicPageEditView.vueassets/vue/views/PublicPagesView.vueassets/vue/views/TemplateEditView.vuecomposer.jsonconfig/packages/framework.yamlconfig/packages/security.yamlconfig/services.ymlopenapi.jsonpackage.jsonsrc/Command/PublishPhplistTextsCommand.phpsrc/Controller/AuthController.phpsrc/Controller/BaseController.phpsrc/Controller/InternalController.phpsrc/Controller/PublicPagesController.phpsrc/Controller/PublicSubscribeController.phpsrc/DependencyInjection/PhpListFrontendExtension.phpsrc/EventSubscriber/AuthGateSubscriber.phpsrc/EventSubscriber/UnauthorizedSubscriber.phpsrc/Security/SessionAuthenticator.phpsrc/Service/AttributeValidator.phpsrc/Service/FormDataMapper.phpsrc/Service/LanguageService.phpsrc/Service/ListSelectionService.phpsrc/Service/PhpListTranslationLoader.phpsrc/Service/PublicSubscribeFormBuilder.phpsrc/Service/PublicSubscribeFormValidator.phpsrc/Twig/HtmlCleanupExtension.phptemplates/auth/login.html.twigtemplates/base.html.twigtemplates/public/base.html.twigtemplates/public/subscribe.html.twigtemplates/public/unsubscribe.html.twigtests/Integration/Controller/PublicPagesControllerPantherTest.phptests/Integration/Controller/PublicSubscribeControllerPantherTest.phptests/Unit/Controller/AuthControllerTest.phptests/Unit/EventSubscriber/AuthGateSubscriberTest.phptests/Unit/EventSubscriber/UnauthorizedSubscriberTest.phptests/Unit/Security/SessionAuthenticatorTest.phptranslations/messages.en.phplisttranslations/messages.es.phplistwebpack.config.js
Comment on lines
+95
to
+96
| class="h-4 w-4 rounded border-slate-300 text-slate-900 accent-ext-wf1" | ||
| data-testid="send-btn" |
There was a problem hiding this comment.
Move data-testid="send-btn" to the actual Send button.
The data-testid="send-btn" is currently on the checkbox input, but the tests in ViewCampaignModal.spec.js (lines 263-288) expect to find it on the "Send to lists" button at lines 107-114. The test checks .element.disabled, which will incorrectly target the checkbox's disabled state instead of the button's.
Move the testid from line 96 to line 107 (the button element).
🧪 Fix for test selector
Remove from checkbox:
<input
:id="`mailing-list-${list.id}`"
v-model="selectedMailingListIds"
type="checkbox"
:value="list.id"
class="h-4 w-4 rounded border-slate-300 text-slate-900 accent-ext-wf1"
- data-testid="send-btn"
>Add to button:
<button
type="button"
+ data-testid="send-btn"
class="inline-flex items-center justify-center rounded-md bg-slate-900 px-4 py-2 text-sm font-medium text-white hover:bg-slate-800 disabled:cursor-not-allowed disabled:opacity-50"
:disabled="selectedMailingListIds.length === 0 || isResending"
`@click`="handleResend"
>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@assets/vue/components/campaigns/ViewCampaignModal.vue` around lines 95 - 96,
The data-testid is placed on the checkbox input (the input element for "send to
lists" option) but tests expect it on the actual "Send to lists" button; remove
data-testid="send-btn" from the checkbox input and add data-testid="send-btn" to
the button element that renders the "Send to lists" action (the button with the
label/text "Send to lists" in ViewCampaignModal.vue) so the tests check the
button's disabled state instead of the checkbox.
Comment on lines
+79
to
+82
| $this->assertSelectorExists('form.legacy-form'); | ||
| $this->assertSelectorExists('input#email[name="email"][type="email"]'); | ||
| $this->assertSelectorExists('button[type="submit"].legacy-button'); | ||
| } |
There was a problem hiding this comment.
Fix brittle selectors causing CI failure.
Line 79 is currently failing in CI because the test expects legacy class hooks that aren’t present in the rendered markup. Use resilient selectors tied to behavior (form/email/submit) instead of legacy styling classes.
Suggested patch
- $this->assertSelectorExists('form.legacy-form');
- $this->assertSelectorExists('input#email[name="email"][type="email"]');
- $this->assertSelectorExists('button[type="submit"].legacy-button');
+ $this->assertSelectorExists('form');
+ $this->assertSelectorExists('input[name="email"][type="email"]');
+ $this->assertSelectorExists('button[type="submit"]');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $this->assertSelectorExists('form.legacy-form'); | |
| $this->assertSelectorExists('input#email[name="email"][type="email"]'); | |
| $this->assertSelectorExists('button[type="submit"].legacy-button'); | |
| } | |
| $this->assertSelectorExists('form'); | |
| $this->assertSelectorExists('input[name="email"][type="email"]'); | |
| $this->assertSelectorExists('button[type="submit"]'); | |
| } |
🧰 Tools
🪛 GitHub Actions: phpList Web Frontend Build / 0_phpList Base Dist on PHP 8.1, with dist [Build, Test].txt
[error] 79-79: PHPUnit/Panther assertion failed in testSubscribePageDisplaysEmailFieldAndSubmitButton: expected an array not to be empty but it was empty. (Failed asserting that an array is not empty.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Integration/Controller/PublicSubscribeControllerPantherTest.php` around
lines 79 - 82, The selectors in the assertions are brittle—replace the legacy
class-based selectors used in assertSelectorExists ('form.legacy-form',
'input#email[name="email"][type="email"]',
'button[type="submit"].legacy-button') with behavior-focused selectors: look for
a form element (e.g. 'form' or 'form[action]' or a form with role), the email
input by attribute only (e.g. 'input[type="email"][name="email"]' without
relying on id/class), and the submit control by type (e.g.
'button[type="submit"], input[type="submit"]'); update the three
assertSelectorExists calls accordingly in the test so they no longer depend on
legacy styling hooks.
Source: Pipeline failures
TatevikGr
force-pushed
the
subscribe-page
branch
5 times, most recently
from
f3a8881 to
747bf9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets/vue/components/subscribers/SubscriberDirectory.spec.js`:
- Around line 53-77: Tests share mutable fixtures subscribers and pagination
causing state leakage; update createWrapper (used to mount SubscriberDirectory)
to return fresh copies by creating new objects inside the function (e.g., clone
the subscribers array and pagination object before providing them) so each mount
gets its own instances, and ensure any test that mutates subscriber objects
operates on those fresh clones.
In `@assets/vue/components/subscribers/SubscriberFilters.spec.js`:
- Around line 124-127: Replace the early-return guard that hides regressions by
asserting the expected minimum number of buttons before interacting with them:
remove the `if (buttons.length < 2) return` check in SubscriberFilters.spec.js
and add an assertion that `buttons.length` is at least 3 (or the exact minimum
required for the test) before calling `buttons[1].trigger('click')` and
`buttons[2].trigger('click')`; make the same change to the other occurrence
around lines 146-147 so the test fails when too few buttons render rather than
silently passing.
In `@assets/vue/components/templates/TemplateLibrary.spec.js`:
- Line 3: The tests globally mock window.confirm in TemplateLibrary.spec.js via
setup (and/or within beforeEach), but they never restore it, causing a global
leak; add an afterEach block (or use afterEach(() => { vi.restoreAllMocks(); }))
that restores window.confirm (e.g., call vi.spyOn(window,
'confirm').mockRestore() or vi.restoreAllMocks()) so the global confirm is reset
after each test in this suite (reference window.confirm, beforeEach, afterEach
in TemplateLibrary.spec.js).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7450a343-7f88-4630-8dbe-d4223f604bf7
📒 Files selected for processing (35)
.github/workflows/ci.ymlassets/vue/components/base/CkEditorField.spec.jsassets/vue/components/bounces/BounceOverview.spec.jsassets/vue/components/bounces/BouncePer.spec.jsassets/vue/components/bounces/BounceRules.spec.jsassets/vue/components/bounces/BounceRules.vueassets/vue/components/bounces/BouncesActionsPanel.spec.jsassets/vue/components/campaigns/CampaignDirectory.spec.jsassets/vue/components/campaigns/ViewCampaignModal.spec.jsassets/vue/components/campaigns/ViewCampaignModal.vueassets/vue/components/dashboard/CampaignsTable.spec.jsassets/vue/components/dashboard/KpiCard.spec.jsassets/vue/components/dashboard/KpiGrid.spec.jsassets/vue/components/dashboard/PerformanceChartCard.spec.jsassets/vue/components/dashboard/QuickActionsCard.spec.jsassets/vue/components/dashboard/RecentCampaignsCard.spec.jsassets/vue/components/lists/AddSubscribersModal.spec.jsassets/vue/components/lists/CreateListModal.spec.jsassets/vue/components/lists/EditListModal.spec.jsassets/vue/components/lists/ListDirectory.spec.jsassets/vue/components/lists/ListSubscribersExportPanel.spec.jsassets/vue/components/sidebar/AppSidebar.spec.jsassets/vue/components/sidebar/SidebarLogo.spec.jsassets/vue/components/sidebar/SidebarNavItem.spec.jsassets/vue/components/sidebar/SidebarNavSection.spec.jsassets/vue/components/subscribers/ImportResult.spec.jsassets/vue/components/subscribers/SubscriberDirectory.spec.jsassets/vue/components/subscribers/SubscriberFilters.spec.jsassets/vue/components/subscribers/SubscriberModal.spec.jsassets/vue/components/subscribers/SubscriberModal.vueassets/vue/components/subscribers/SubscriberTable.spec.jsassets/vue/components/templates/TemplateLibrary.spec.jsassets/vue/layouts/AdminLayout.spec.jstests/Integration/Controller/ListsControllerPantherTest.phptests/Integration/Controller/PublicSubscribeControllerPantherTest.php
✅ Files skipped from review due to trivial changes (2)
- assets/vue/components/bounces/BounceRules.vue
- assets/vue/components/dashboard/KpiCard.spec.js
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/ci.yml
- assets/vue/components/bounces/BounceRules.spec.js
- tests/Integration/Controller/PublicSubscribeControllerPantherTest.php
- assets/vue/components/campaigns/ViewCampaignModal.spec.js
- assets/vue/components/bounces/BouncePer.spec.js
- assets/vue/components/bounces/BouncesActionsPanel.spec.js
- assets/vue/components/campaigns/CampaignDirectory.spec.js
- assets/vue/components/bounces/BounceOverview.spec.js
Comment on lines
+53
to
+77
| const subscribers = [ | ||
| { | ||
| id: 1, | ||
| email: 'john@example.com', | ||
| confirmed: true, | ||
| blacklisted: false, | ||
| listCount: 2, | ||
| }, | ||
| ] | ||
|
|
||
| const pagination = { | ||
| total: 1, | ||
| isFirstPage: true, | ||
| hasMore: false, | ||
| afterId: null, | ||
| prevId: null, | ||
| } | ||
|
|
||
| const createWrapper = () => | ||
| mount(SubscriberDirectory, { | ||
| global: { | ||
| provide: { | ||
| subscribers, | ||
| pagination, | ||
| }, |
There was a problem hiding this comment.
Use fresh fixture objects per test to avoid state leakage.
subscribers/pagination are shared by reference across all tests. The update path can mutate subscriber data, making later tests order-dependent.
Suggested tweak
- const subscribers = [
- {
- id: 1,
- email: 'john@example.com',
- confirmed: true,
- blacklisted: false,
- listCount: 2,
- },
- ]
-
- const pagination = {
- total: 1,
- isFirstPage: true,
- hasMore: false,
- afterId: null,
- prevId: null,
- }
+ const createSubscribers = () => ([
+ {
+ id: 1,
+ email: 'john@example.com',
+ confirmed: true,
+ blacklisted: false,
+ listCount: 2,
+ },
+ ])
+
+ const createPagination = () => ({
+ total: 1,
+ isFirstPage: true,
+ hasMore: false,
+ afterId: null,
+ prevId: null,
+ })
const createWrapper = () =>
mount(SubscriberDirectory, {
global: {
provide: {
- subscribers,
- pagination,
+ subscribers: createSubscribers(),
+ pagination: createPagination(),
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@assets/vue/components/subscribers/SubscriberDirectory.spec.js` around lines
53 - 77, Tests share mutable fixtures subscribers and pagination causing state
leakage; update createWrapper (used to mount SubscriberDirectory) to return
fresh copies by creating new objects inside the function (e.g., clone the
subscribers array and pagination object before providing them) so each mount
gets its own instances, and ensure any test that mutates subscriber objects
operates on those fresh clones.
Comment on lines
+124
to
+127
| if (buttons.length < 2) return | ||
|
|
||
| await buttons[1].trigger('click') | ||
| await buttons[2].trigger('click') |
There was a problem hiding this comment.
Avoid early-return guards that hide regressions.
These return guards let the test pass when too few buttons render. Assert the minimum count instead so the suite fails on real UI breakage.
Suggested tweak
- if (buttons.length < 2) return
+ expect(buttons.length).toBeGreaterThan(2)
...
- if (buttons.length < 2) return
+ expect(buttons.length).toBeGreaterThan(2)Also applies to: 146-147
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@assets/vue/components/subscribers/SubscriberFilters.spec.js` around lines 124
- 127, Replace the early-return guard that hides regressions by asserting the
expected minimum number of buttons before interacting with them: remove the `if
(buttons.length < 2) return` check in SubscriberFilters.spec.js and add an
assertion that `buttons.length` is at least 3 (or the exact minimum required for
the test) before calling `buttons[1].trigger('click')` and
`buttons[2].trigger('click')`; make the same change to the other occurrence
around lines 146-147 so the test fails when too few buttons render rather than
silently passing.
| @@ -0,0 +1,458 @@ | |||
| // TemplateLibrary.spec.js | |||
|
|
|||
| import { describe, it, expect, beforeEach, vi } from 'vitest' | |||
There was a problem hiding this comment.
Restore window.confirm after each test to avoid global leak across suites.
window.confirm is globally mocked in setup but not restored, so later test files can inherit this mocked behavior unexpectedly.
🔧 Suggested fix
-import { describe, it, expect, beforeEach, vi } from 'vitest'
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'
...
describe('TemplateLibrary', () => {
+ const originalConfirm = window.confirm
+
beforeEach(() => {
vi.clearAllMocks()
window.confirm = vi.fn()
...
})
+
+ afterEach(() => {
+ window.confirm = originalConfirm
+ })Also applies to: 31-34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@assets/vue/components/templates/TemplateLibrary.spec.js` at line 3, The tests
globally mock window.confirm in TemplateLibrary.spec.js via setup (and/or within
beforeEach), but they never restore it, causing a global leak; add an afterEach
block (or use afterEach(() => { vi.restoreAllMocks(); })) that restores
window.confirm (e.g., call vi.spyOn(window, 'confirm').mockRestore() or
vi.restoreAllMocks()) so the global confirm is reset after each test in this
suite (reference window.confirm, beforeEach, afterEach in
TemplateLibrary.spec.js).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes / UI
Tests
Summary
Provide a general description of the code changes in your pull request …
were there any bugs you had fixed? If so, mention them. If these bugs have open
GitHub issues, be sure to tag them here as well, to keep the conversation
linked together.
Unit test
Are your changes covered with unit tests, and do they not break anything?
You can run the existing unit tests using this command:
Code style
Have you checked that you code is well-documented and follows the PSR-2 coding
style?
You can check for this using this command:
Other Information
If there's anything else that's important and relevant to your pull
request, mention that information here. This could include benchmarks,
or other information.
If you are updating any of the CHANGELOG files or are asked to update the
CHANGELOG files by reviewers, please add the CHANGELOG entry at the top of the file.
Thanks for contributing to phpList!