Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a complete bounces management feature: new frontend routes, view, and components (Overview, Rules, Per, ActionsPanel) plus a exported Sequence DiagramsequenceDiagram
actor User
participant Router as Vue Router
participant View as BouncesView
participant Panel as BouncesActionsPanel
participant Component as Bounce Component
participant Client as bouncesClient / backendFetch
participant API as Backend API
User->>Router: Navigate to /bounces
Router->>View: route match
View->>Panel: render
Panel->>Component: mount active tab
alt Overview
Component->>Client: list(after, limit, status)
Client->>API: GET /api/v2/bounces
API-->>Client: paginated bounces
Client-->>Component: bounces
Component-->>User: render list / pagination
else Rules
Component->>Client: listRegex()
Client->>API: GET /api/v2/bounces/regex
API-->>Client: rules
Client-->>Component: rules
User->>Component: submit rule
Component->>Client: upsertRegex(payload)
Client->>API: POST /api/v2/bounces/regex
API-->>Client: success / validation error
Client-->>Component: response
Component->>Client: listRegex() (refresh)
else Per
par
Component->>Client: getByCampaign()
Component->>Client: getBySubscriber()
and
Client->>API: GET /api/v2/bounces/by-campaign
Client->>API: GET /api/v2/bounces/by-subscriber
end
API-->>Client: aggregates
Client-->>Component: datasets
Component-->>User: render aggregates
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 44 minutes and 34 seconds.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: 6
🧹 Nitpick comments (3)
.gitignore (1)
20-20: 💤 Low valueMake the
.codexignore pattern explicit (root-only vs anywhere).Right now
.gitignoreuses.codex(no leading/, no trailing/), which will match any file/dir named.codexat any depth in the repo. If you only intend to ignore the tool-generated directory at the repo root, consider tightening it to something like/.codex/(or.codex/if it can exist in subfolders but only as a directory).♻️ Proposed tweak
- .codex + /.codex/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 20, Update the .gitignore entry for the Codex artifact: replace the current ".codex" pattern with a root-only directory pattern "/.codex/" (or "/.codex" if you also want to ignore a root file) so the ignore applies only to the repo-root .codex directory/file; modify the line in .gitignore that currently contains ".codex" accordingly.assets/vue/components/bounces/BounceRules.vue (1)
318-329: ⚡ Quick winSurface list-load failures in the UI.
A non-auth failure currently becomes
allBounceRules = []plus a console log, and this template has no loading/error/empty state. In practice that makes “fetch failed” look the same as “there are no rules”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vue/components/bounces/BounceRules.vue` around lines 318 - 329, The loadBounceRules function currently swallows non-auth errors and sets allBounceRules to an empty array, which makes failures indistinguishable from an empty result; add explicit UI state and wiring: introduce reactive flags (e.g., bounceRulesLoading and bounceRulesError) alongside allBounceRules, set bounceRulesLoading=true at start and false in finally, set bounceRulesError to the caught error (except when redirecting for AuthenticationException/status 401), and only set allBounceRules to [] on success or when there truly are no rules; update the component template to render a loading indicator when bounceRulesLoading, an error message when bounceRulesError is set, and the existing empty-state when allBounceRules is empty and bounceRulesError is null.tests/Integration/Controller/BouncesControllerTest.php (1)
39-43: ⚡ Quick winAssert the new
data-bounce-actionspayload too.The route/title/token assertions all pass even if the
BounceAction::cases()mapping regresses, which is the feature-specific part of this controller. It’d be good to validate that the rendered SPA payload actually includesdata-bounce-actions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Integration/Controller/BouncesControllerTest.php` around lines 39 - 43, The test currently checks status, title, token and base URL but misses asserting the SPA payload for bounce actions; update the BouncesControllerTest assertions to also verify the rendered HTML contains the data-bounce-actions attribute (and ideally its value matches the current mapping from BounceAction::cases()). Concretely, add an assertion against $content such as asserting presence of 'data-bounce-actions="' and compare the attribute value to json_encode(array_values(BounceAction::cases())) (or otherwise decode/extract the attribute and assert it equals the expected BounceAction::cases() payload) so regressions in BounceAction::cases() are caught.
🤖 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/bounces/BounceOverview.vue`:
- Around line 223-248: loadBounces currently iterates up to 100 backend pages
then slices to client pages; change it to fetch only the current client page by
keeping cursor state in the component (e.g., currentPage, pageSize, and a
cursors map like pageCursors) and call bouncesClient.list(afterCursor, pageSize,
selectedStatus.value) once per page instead of the for-loop in loadBounces;
update loadBounces (and pagination controls) to compute the afterCursor for the
requested page from pageCursors (or request subsequent pages incrementally and
store their nextCursor), assign the returned items directly to
allBounces/currentPageItems, and keep existing isLoading/errorMessage handling
while removing the large loop and client-side slicing logic.
- Around line 223-266: Concurrent loadBounces calls can overwrite allBounces
with stale data; add a request guard: create a module-level ref (e.g.,
currentRequestId) that you increment each time loadBounces starts, capture the
incremented id in a local variable inside loadBounces and only assign to
allBounces.value, isLoading.value and errorMessage.value if the captured id
still equals currentRequestId; alternatively, if bouncesClient.list supports
cancellation, create and store an AbortController per request and abort the
previous controller before starting a new request—apply this change around the
loadBounces function and where watch(statusFilter) calls it so stale responses
are ignored.
In `@assets/vue/components/bounces/BouncePer.vue`:
- Around line 198-205: The catch block for the bouncesClient.listByCampaign call
currently only sets campaignErrorMessage and bouncesPerCampaign; change it to
detect HTTP 401 responses and trigger the same logout/redirect path used by the
other bounces flows (e.g., call the shared logout/unauthorized handler or router
redirect used elsewhere) instead of just showing an error. Specifically, in the
catch for bouncesClient.listByCampaign (and the similar catch at the 213-220
block), inspect the caught error for a 401 (error.response?.status === 401 or
error?.status === 401) and invoke the existing logout/redirect function (the
same symbol used in the other bounces flows) to end the session; otherwise
continue setting campaignErrorMessage and clearing bouncesPerCampaign and
isLoadingCampaigns as before.
In `@assets/vue/components/bounces/BounceRules.vue`:
- Around line 244-250: createForm.action is left empty so when the form is
untouched payload.action is omitted and backend defaults are used; set
createForm.action to a sensible default (e.g. the first entry of your
actions/options list) or replace the empty string with a placeholder value and
enforce selection before submit so payload.action is always present. Update the
same pattern used in the other form object around lines 363-365 (the second form
declaration) so both createForm and the other form seed their .action field from
your actions/options array (or require explicit choice) before building
payload.action.
- Around line 388-397: When creating a rule via bouncesClient.upsertRegex fails
due to expired auth, the catch block should detect a 401 and trigger the same
logout/redirect path used by loadBounceRules instead of falling back to the
generic create error handling; update the catch in the upsertRegex call to check
the error response/status for 401 and call the same handler (or invoke
loadBounceRules) so the user is redirected, otherwise continue to set
createFieldErrors via normalizeValidationErrors and set createError as before
(referencing bouncesClient.upsertRegex, normalizeValidationErrors,
createFieldErrors, createError, and loadBounceRules).
In `@tests/Integration/Controller/BouncesControllerTest.php`:
- Line 42: The test is pinned to the literal string 'http://api.phplist.local/'
— update the assertion in BouncesControllerTest (the line using
assertStringContainsString for data-api-base-url) to read the expected URL from
the test container parameter instead of the hardcoded literal; fetch the
configured value (e.g. via self::$container->getParameter('API_BASE_URL') or the
project's configured parameter name) and use that variable in the
assertStringContainsString so the test respects overridden API_BASE_URL in the
test environment.
---
Nitpick comments:
In @.gitignore:
- Line 20: Update the .gitignore entry for the Codex artifact: replace the
current ".codex" pattern with a root-only directory pattern "/.codex/" (or
"/.codex" if you also want to ignore a root file) so the ignore applies only to
the repo-root .codex directory/file; modify the line in .gitignore that
currently contains ".codex" accordingly.
In `@assets/vue/components/bounces/BounceRules.vue`:
- Around line 318-329: The loadBounceRules function currently swallows non-auth
errors and sets allBounceRules to an empty array, which makes failures
indistinguishable from an empty result; add explicit UI state and wiring:
introduce reactive flags (e.g., bounceRulesLoading and bounceRulesError)
alongside allBounceRules, set bounceRulesLoading=true at start and false in
finally, set bounceRulesError to the caught error (except when redirecting for
AuthenticationException/status 401), and only set allBounceRules to [] on
success or when there truly are no rules; update the component template to
render a loading indicator when bounceRulesLoading, an error message when
bounceRulesError is set, and the existing empty-state when allBounceRules is
empty and bounceRulesError is null.
In `@tests/Integration/Controller/BouncesControllerTest.php`:
- Around line 39-43: The test currently checks status, title, token and base URL
but misses asserting the SPA payload for bounce actions; update the
BouncesControllerTest assertions to also verify the rendered HTML contains the
data-bounce-actions attribute (and ideally its value matches the current mapping
from BounceAction::cases()). Concretely, add an assertion against $content such
as asserting presence of 'data-bounce-actions="' and compare the attribute value
to json_encode(array_values(BounceAction::cases())) (or otherwise decode/extract
the attribute and assert it equals the expected BounceAction::cases() payload)
so regressions in BounceAction::cases() are caught.
🪄 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: bbfe6401-dd8f-4694-8641-98a5953700ba
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
.gitignoreassets/router/index.jsassets/vue/api.jsassets/vue/components/bounces/BounceOverview.vueassets/vue/components/bounces/BouncePer.vueassets/vue/components/bounces/BounceRules.vueassets/vue/components/bounces/BouncesActionsPanel.vueassets/vue/views/BouncesView.vuecomposer.jsonpackage.jsonsrc/Controller/BouncesController.phpsrc/DependencyInjection/PhpListFrontendExtension.phptemplates/spa.html.twigtests/Integration/Controller/BouncesControllerTest.php
Comment on lines
+198
to
+205
| try { | ||
| const bounces = await bouncesClient.listByCampaign() | ||
| bouncesPerCampaign.value = Array.isArray(bounces) ? bounces : [] | ||
| } catch (error) { | ||
| campaignErrorMessage.value = error?.message ?? 'Failed to load campaign bounce data.' | ||
| bouncesPerCampaign.value = [] | ||
| } finally { | ||
| isLoadingCampaigns.value = false |
There was a problem hiding this comment.
Treat API 401s as a logout path here too.
If the session expires, this panel just renders an error message and leaves the user stranded. The other bounces flows already redirect on 401, so this view should do the same instead of requiring a manual reload.
Possible fix
try {
const bounces = await bouncesClient.listByCampaign()
bouncesPerCampaign.value = Array.isArray(bounces) ? bounces : []
} catch (error) {
+ if (error?.name === 'AuthenticationException' || error?.status === 401) {
+ window.location.href = '/login'
+ return
+ }
campaignErrorMessage.value = error?.message ?? 'Failed to load campaign bounce data.'
bouncesPerCampaign.value = []
} finally {
isLoadingCampaigns.value = false
}
@@
try {
const bounces = await bouncesClient.listBySubscriber()
bouncesPerSubscriber.value = Array.isArray(bounces) ? bounces : []
} catch (error) {
+ if (error?.name === 'AuthenticationException' || error?.status === 401) {
+ window.location.href = '/login'
+ return
+ }
subscriberErrorMessage.value = error?.message ?? 'Failed to load subscriber bounce data.'
bouncesPerSubscriber.value = []
} finally {
isLoadingSubscribers.value = false
}Also applies to: 213-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/bounces/BouncePer.vue` around lines 198 - 205, The
catch block for the bouncesClient.listByCampaign call currently only sets
campaignErrorMessage and bouncesPerCampaign; change it to detect HTTP 401
responses and trigger the same logout/redirect path used by the other bounces
flows (e.g., call the shared logout/unauthorized handler or router redirect used
elsewhere) instead of just showing an error. Specifically, in the catch for
bouncesClient.listByCampaign (and the similar catch at the 213-220 block),
inspect the caught error for a 401 (error.response?.status === 401 or
error?.status === 401) and invoke the existing logout/redirect function (the
same symbol used in the other bounces flows) to end the session; otherwise
continue setting campaignErrorMessage and clearing bouncesPerCampaign and
isLoadingCampaigns as before.
Comment on lines
+388
to
+397
| try { | ||
| await bouncesClient.upsertRegex(payload) | ||
| isCreateModalOpen.value = false | ||
| await loadBounceRules() | ||
| } catch (error) { | ||
| createFieldErrors.value = normalizeValidationErrors(error) | ||
| createError.value = Object.keys(createFieldErrors.value).length > 0 | ||
| ? '' | ||
| : error?.message ?? 'Failed to create rule.' | ||
| } finally { |
There was a problem hiding this comment.
Redirect on expired auth during rule creation.
loadBounceRules() already treats 401 as a logout path, but create falls through to a generic error. If the token expires while the modal is open, the user is left on a form that can’t recover without a manual refresh.
Possible fix
try {
await bouncesClient.upsertRegex(payload)
isCreateModalOpen.value = false
await loadBounceRules()
} catch (error) {
+ if (error?.name === 'AuthenticationException' || error?.status === 401) {
+ window.location.href = '/login'
+ return
+ }
+
createFieldErrors.value = normalizeValidationErrors(error)
createError.value = Object.keys(createFieldErrors.value).length > 0
? ''
: error?.message ?? 'Failed to create rule.'📝 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
| try { | |
| await bouncesClient.upsertRegex(payload) | |
| isCreateModalOpen.value = false | |
| await loadBounceRules() | |
| } catch (error) { | |
| createFieldErrors.value = normalizeValidationErrors(error) | |
| createError.value = Object.keys(createFieldErrors.value).length > 0 | |
| ? '' | |
| : error?.message ?? 'Failed to create rule.' | |
| } finally { | |
| try { | |
| await bouncesClient.upsertRegex(payload) | |
| isCreateModalOpen.value = false | |
| await loadBounceRules() | |
| } catch (error) { | |
| if (error?.name === 'AuthenticationException' || error?.status === 401) { | |
| window.location.href = '/login' | |
| return | |
| } | |
| createFieldErrors.value = normalizeValidationErrors(error) | |
| createError.value = Object.keys(createFieldErrors.value).length > 0 | |
| ? '' | |
| : error?.message ?? 'Failed to create rule.' | |
| } finally { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/bounces/BounceRules.vue` around lines 388 - 397, When
creating a rule via bouncesClient.upsertRegex fails due to expired auth, the
catch block should detect a 401 and trigger the same logout/redirect path used
by loadBounceRules instead of falling back to the generic create error handling;
update the catch in the upsertRegex call to check the error response/status for
401 and call the same handler (or invoke loadBounceRules) so the user is
redirected, otherwise continue to set createFieldErrors via
normalizeValidationErrors and set createError as before (referencing
bouncesClient.upsertRegex, normalizeValidationErrors, createFieldErrors,
createError, and loadBounceRules).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
openapi.json (3)
3469-3486:⚠️ Potential issue | 🔴 Critical
PUT /api/v2/templates/{templateId}is missing the required path parameter declaration.This operation uses
{templateId}in the path but doesn't declare it inparameters. The GET operation on the same path correctly includes it, but PUT is missing it. This violates OpenAPI 3.0 specs and will cause strict validators and code generators to fail.🔧 Proposed fix
"put": { "tags": [ "templates" ], "summary": "Update template.", @@ "parameters": [ { "name": "php-auth-pw", "in": "header", "description": "Session key obtained from login", "required": true, "schema": { "type": "string" } + }, + { + "name": "templateId", + "in": "path", + "description": "Template ID", + "required": true, + "schema": { + "type": "string" + } } ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openapi.json` around lines 3469 - 3486, The PUT operation for the path that contains {templateId} (operationId a73b9801869083ca7a9739aed68d1b92) is missing the required path parameter declaration; add a parameters entry for "templateId" with "in": "path", "required": true and an appropriate schema (e.g., type: "string" or the same type used by the GET operation for this path) so the PUT matches the path template and OpenAPI validation/codegen succeeds.
1868-1874:⚠️ Potential issue | 🟠 MajorFix
ruleIdtype inconsistency in DELETE bounce regex operation.The
ruleIdparameter usesintegerfor both GET and PUT operations, butstringfor DELETE. This mismatch breaks generated clients and validation logic. Update the DELETE operation to match.Suggested fix
{ "name": "ruleId", "in": "path", - "description": "Regex hash", + "description": "Regex rule ID", "required": true, "schema": { - "type": "string" + "type": "integer" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openapi.json` around lines 1868 - 1874, Update the DELETE operation's path parameter "ruleId" to use the same integer type as GET and PUT: locate the DELETE /bounce/regex/{ruleId} operation's parameter named "ruleId" and change its schema type from "string" to "integer" (preserving any format if used), ensuring required and description remain unchanged so generated clients and validation match the other operations.
1-20:⚠️ Potential issue | 🟠 MajorAuth should be defined as a reusable security scheme instead of duplicated header params.
The spec repeats
php-auth-pwas a header parameter across 82+ operations instead of usingcomponents.securitySchemes. Define a security scheme and apply it globally or per-operation to reduce duplication, improve client generation, and enable policy checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openapi.json` around lines 1 - 20, Replace the repeated "php-auth-pw" header parameter with a reusable security scheme in components.securitySchemes (e.g., name it "PhpAuthPassword" or "phpAuthPw") that models the header-based auth (type: http or apiKey in: header with name "php-auth-pw"), then remove the duplicated header parameter entries from individual operations and reference the scheme globally by adding a top-level "security" array that uses the new scheme (or apply per-operation where needed); update any operation examples/parameters that referenced "php-auth-pw" to rely on the security requirement instead.
🤖 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/bounces/BounceOverview.vue`:
- Around line 104-117: The Next/Previous buttons and their handlers (nextPage,
previousPage) allow navigation while a request is in flight, causing undefined
cursor fetches; update the template button :disabled bindings to include
isLoading (e.g., :disabled="!canGoNext || isLoading") and in the methods
nextPage and previousPage add early returns when isLoading is true and verify
the existence of the relevant cursor (e.g., nextCursor/prevCursor) before
mutating currentPage or invoking the fetch; this ensures navigation is gated
until the current request completes and a valid cursor is available.
In `@openapi.json`:
- Around line 3252-3261: The OpenAPI responses incorrectly return an array and
use the wrong status for updates: change the response schemas for the
single-template endpoints to return a single Template object (remove "type:
array" and the "items" wrapper so the schema is a direct $ref to
"#/components/schemas/Template") and update the PUT endpoint's response status
from "201" to "200" (with description "Success" or "OK"); locate the response
blocks for the PUT and other single-template endpoints in the spec and apply
these two changes.
- Around line 1355-1401: The 200 response for the Bounces list currently returns
a raw array of BounceView items while the operation accepts cursor pagination
params (after_id, limit), causing inconsistency with other endpoints; change the
response schema for the 200 success to an object with two properties: "items"
(array of items with "$ref": "#/components/schemas/BounceView") and "pagination"
(a "$ref" to the existing CursorPagination schema), matching the
/api/v2/subscribers pattern so clients receive both data and pagination
metadata.
---
Outside diff comments:
In `@openapi.json`:
- Around line 3469-3486: The PUT operation for the path that contains
{templateId} (operationId a73b9801869083ca7a9739aed68d1b92) is missing the
required path parameter declaration; add a parameters entry for "templateId"
with "in": "path", "required": true and an appropriate schema (e.g., type:
"string" or the same type used by the GET operation for this path) so the PUT
matches the path template and OpenAPI validation/codegen succeeds.
- Around line 1868-1874: Update the DELETE operation's path parameter "ruleId"
to use the same integer type as GET and PUT: locate the DELETE
/bounce/regex/{ruleId} operation's parameter named "ruleId" and change its
schema type from "string" to "integer" (preserving any format if used), ensuring
required and description remain unchanged so generated clients and validation
match the other operations.
- Around line 1-20: Replace the repeated "php-auth-pw" header parameter with a
reusable security scheme in components.securitySchemes (e.g., name it
"PhpAuthPassword" or "phpAuthPw") that models the header-based auth (type: http
or apiKey in: header with name "php-auth-pw"), then remove the duplicated header
parameter entries from individual operations and reference the scheme globally
by adding a top-level "security" array that uses the new scheme (or apply
per-operation where needed); update any operation examples/parameters that
referenced "php-auth-pw" to rely on the security requirement instead.
🪄 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: aa60ad07-4321-4d09-af2c-8797cc27eef5
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
assets/vue/api.jsassets/vue/components/bounces/BounceOverview.vueassets/vue/components/bounces/BounceRules.vueassets/vue/components/subscribers/SubscriberDirectory.vueassets/vue/layouts/AdminLayout.vuecomposer.jsonopenapi.jsonpackage.jsonsrc/DependencyInjection/PhpListFrontendExtension.phptests/Integration/Controller/BouncesControllerTest.phptests/Integration/Controller/DashboardControllerTest.phptests/Integration/Controller/ListsControllerTest.php
✅ Files skipped from review due to trivial changes (4)
- package.json
- composer.json
- tests/Integration/Controller/DashboardControllerTest.php
- src/DependencyInjection/PhpListFrontendExtension.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Integration/Controller/BouncesControllerTest.php
- assets/vue/components/bounces/BounceRules.vue
Comment on lines
+104
to
+117
| <button | ||
| type="button" | ||
| class="flex-1 sm:flex-none px-4 py-2 border border-slate-200 rounded-lg hover:bg-slate-50 transition-colors disabled:opacity-50" | ||
| :disabled="!canGoPrevious" | ||
| @click="previousPage" | ||
| > | ||
| Previous | ||
| </button> | ||
| <button | ||
| type="button" | ||
| class="flex-1 sm:flex-none px-4 py-2 border border-slate-200 rounded-lg hover:bg-slate-50 transition-colors disabled:opacity-50" | ||
| :disabled="!canGoNext" | ||
| @click="nextPage" | ||
| > |
There was a problem hiding this comment.
Prevent pagination jumps while a page request is still in flight.
On Line 115 + Lines 199-205, Next can be clicked again before the current request finishes. That can advance currentPage with an undefined cursor and fetch the wrong dataset (often resetting to first-page cursor behavior). Gate navigation by isLoading and ensure a next cursor exists before advancing.
Suggested fix
- :disabled="!canGoPrevious"
+ :disabled="!canGoPrevious || isLoading"
@@
- :disabled="!canGoNext"
+ :disabled="!canGoNext || isLoading"
@@
const nextPage = () => {
- if (!canGoNext.value) return
+ if (!canGoNext.value || isLoading.value) return
+
+ const nextCursor = pageCursors.value[currentPage.value]
+ if (nextCursor === undefined) return
currentPage.value += 1
- currentCursor.value = pageCursors.value[currentPage.value - 1] ?? null
+ currentCursor.value = nextCursor ?? null
loadBounces()
}Also applies to: 199-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/vue/components/bounces/BounceOverview.vue` around lines 104 - 117, The
Next/Previous buttons and their handlers (nextPage, previousPage) allow
navigation while a request is in flight, causing undefined cursor fetches;
update the template button :disabled bindings to include isLoading (e.g.,
:disabled="!canGoNext || isLoading") and in the methods nextPage and
previousPage add early returns when isLoading is true and verify the existence
of the relevant cursor (e.g., nextCursor/prevCursor) before mutating currentPage
or invoking the fetch; this ensures navigation is gated until the current
request completes and a valid cursor is available.
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
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!