Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Sequence Diagram(s)sequenceDiagram
participant Browser
participant SettingsController
participant spa_html_twig as spa.html.twig
participant VueRouter
participant SettingsActionsPanel
participant configClient
participant adminClient
participant adminAttributeClient
Browser->>SettingsController: GET /settings
SettingsController->>spa_html_twig: render SPA shell
Browser->>VueRouter: resolve /settings
VueRouter->>SettingsActionsPanel: mount settings UI
User->>SettingsActionsPanel: switch tabs
SettingsActionsPanel->>configClient: load/update configs
SettingsActionsPanel->>adminClient: fetch/create/update/delete admins
SettingsActionsPanel->>adminAttributeClient: fetch/create/update/delete attributes
sequenceDiagram
participant Browser
participant DashboardController
participant spa_html_twig as spa.html.twig
participant DashboardView
Browser->>DashboardController: GET dashboard page
DashboardController->>DashboardController: fetch stats or capture auth error
DashboardController->>spa_html_twig: render dashboard_error
Browser->>DashboardView: mount with data-dashboard-error
DashboardView-->>Browser: show alert when error is present
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/settings/SettingsActionsPanel.vue`:
- Around line 15-18: Replace the deprecated Tailwind utility class
`flex-shrink-0` with the current Tailwind v4 convention `shrink-0` in the class
binding of the SettingsActionsPanel.vue component. The class is located in the
dynamic class attribute that changes based on the activeTab condition, so update
the static class string to use the new utility name for consistency with current
Tailwind standards.
In `@assets/vue/components/settings/SettingsConfigs.vue`:
- Around line 27-71: The SettingsConfigs.vue component has isLoading and error
state variables defined in the script but they are not rendered in the template,
causing users to see a misleading "No configuration keys found" message during
fetch failures or loading. Update the template section to add conditional
rendering that checks isLoading and error states before rendering the
configuration items grid. Add a loading indicator or message when isLoading is
true, display the error message when error is set, and only show the "No
configuration keys found" message when filtered.length is empty and there is no
loading or error state. Use v-if directives to control the visibility of these
different states in the appropriate order.
- Around line 40-44: The input element using v-model="edited[item.key]" lacks an
associated label, making it inaccessible to screen reader users. Add a label
element that explicitly identifies the configuration setting being edited.
Either wrap the input with a label element or add a separate label with a for
attribute pointing to an id on the input. The label text should reference
item.key or the setting name so users can identify which configuration value
they are editing.
🪄 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: 957e3370-b693-425a-90e6-9931b2c1cbf6
📒 Files selected for processing (7)
assets/router/index.jsassets/vue/components/settings/SettingsActionsPanel.vueassets/vue/components/settings/SettingsAdminAttributes.vueassets/vue/components/settings/SettingsAdmins.vueassets/vue/components/settings/SettingsConfigs.vueassets/vue/views/SettingsView.vuesrc/Controller/SettingsController.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/settings/SettingsAdmins.vue`:
- Around line 208-222: The confirmation dialog in the handleDelete function
references admin.login_name, but the admin object uses camelCase properties as
evidenced by the template usage of admin.loginName. Change the property
reference from admin.login_name to admin.loginName in the window.confirm message
to ensure the confirmation dialog displays the actual administrator login name
instead of undefined.
🪄 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: c8984b02-b139-45f8-b66f-6d49d1e8ae26
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
assets/vue/api.jsassets/vue/components/bounces/BouncesActionsPanel.vueassets/vue/components/settings/CreateAdminModal.vueassets/vue/components/settings/EditAdminModal.vueassets/vue/components/settings/SettingsActionsPanel.vueassets/vue/components/settings/SettingsAdminAttributes.vueassets/vue/components/settings/SettingsAdmins.vueassets/vue/components/settings/SettingsConfigs.vueassets/vue/views/SettingsView.vuepackage.json
✅ Files skipped from review due to trivial changes (2)
- assets/vue/components/bounces/BouncesActionsPanel.vue
- assets/vue/views/SettingsView.vue
🚧 Files skipped from review as they are similar to previous changes (2)
- assets/vue/components/settings/SettingsAdminAttributes.vue
- assets/vue/components/settings/SettingsActionsPanel.vue
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/settings/EditAdminModal.vue`:
- Around line 17-18: The close button in EditAdminModal.vue contains only an SVG
icon without a text label or aria-label, making it inaccessible to screen
readers. Add an aria-label attribute to the button element that triggers the
close method to provide an accessible name. The button should have aria-label
set to a descriptive value like "Close" so assistive technology users can
understand the button's purpose.
- Around line 211-218: The watch function currently only observes the isOpen
prop, causing the form to retain stale values when a different admin is selected
while the modal is already open. Modify the watch function to observe both
props.isOpen and props.admin as dependencies, and ensure resetForm() is called
whenever either the modal opens (isOpen becomes true) or the admin selection
changes, preventing stale data from being submitted for the wrong admin.
🪄 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: 90cc9b2e-5936-44e2-92b5-f1b9230d5f2f
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
assets/vue/components/settings/CreateAdminModal.vueassets/vue/components/settings/EditAdminModal.vueassets/vue/components/settings/SettingsAdmins.vuepackage.json
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- assets/vue/components/settings/SettingsAdmins.vue
- assets/vue/components/settings/CreateAdminModal.vue
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
assets/vue/components/settings/CreateAdminAttributeModal.vue (1)
13-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTiny a11y nicety: label the close control.
The
✕button reads as a literal glyph to screen readers. A quickaria-labelmakes the modal friendlier to navigate.♿ Suggested tweak
<button + type="button" + aria-label="Close" class="text-slate-400 hover:text-slate-700" `@click`="$emit('close')" >🤖 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/settings/CreateAdminAttributeModal.vue` around lines 13 - 18, The close control in CreateAdminAttributeModal.vue is currently exposed only as a glyph, so screen readers won’t know its purpose. Update the button that emits the close event to include a clear accessible label using an aria-label on the same button element. Keep the existing click behavior unchanged and make sure the label describes that it closes the modal.assets/vue/components/campaigns/CampaignDirectory.vue (1)
739-808: 🩺 Stability & Availability | 🔵 TrivialSolid auth handling — just tighten the 403 guard and consider isolating stats errors
Add
error?.response?.status === 403to the auth check.
You currently checkerror?.status, butassets/vue/api.js(line 23) establisheserror?.response?.statusas the canonical field for HTTP status codes in this SPA. If the client wraps 403s there, your check misses it, the exception falls through tothrow error, and the entire campaigns list vanishes.Add this to the condition:
|| error?.response?.status === 403Optionally, let stats glitches not trash the campaigns list.
SincefetchCampaignStatisticsruns inside aPromise.allinfetchCampaigns, any non-auth error you rethrow collapses the wholeallCampaignsresult—even if the campaigns themselves loaded perfectly. Now that auth errors already degrade gracefully, you might want other stats errors to do the same (hide stats, keep the list) instead of nucking the page.🤖 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.vue` around lines 739 - 808, The auth guard in the campaign statistics fetch is missing the wrapped HTTP status shape used by the API client. Update the catch block in fetchCampaignStatistics within CampaignDirectory.vue to treat error?.response?.status === 403 the same as the existing AuthorizationException checks, and then keep the fallback behavior of clearing statisticsByCampaignId and hiding showStatistics. If you want to avoid losing the campaigns list on non-auth stats failures, handle those errors locally in fetchCampaignStatistics instead of rethrowing them so fetchCampaigns and Promise.all can still succeed.src/Controller/DashboardController.php (1)
27-32: 🩺 Stability & Availability | 🔵 TrivialHeads up: only auth/authz failures land in the banner.
The
catchblock only handlesAuthenticationException | AuthorizationException. Most HTTP client libraries (like the one backingStatisticsClient) also throw separate exceptions for network blips, 5xx responses, or malformed payloads. Those would bubble up and show a generic error page instead of your friendly dashboard banner.If that narrow scope is intentional, no worries. Otherwise, consider widening the catch to handle general failures gracefully alongside auth issues.
Wish I could grep the
PhpList\RestApiClientsource for exact exception types, but it's not in the sandbox. A quick look at the library docs or your localvendor/folder should confirm what slips past the current catch.Original code context
try { $stats = $this->statisticsClient->getDashboardStats(); $dashboardStats = $this->buildDashboardStats($stats); } catch (AuthenticationException | AuthorizationException $e) { $dashboardError = $e->getMessage() ?: 'Unable to load dashboard statistics.'; }🤖 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/DashboardController.php` around lines 27 - 32, The dashboard stats fetch in DashboardController::show currently catches only AuthenticationException and AuthorizationException, so other client failures can still break the page. Broaden the exception handling around StatisticsClient::getDashboardStats() and buildDashboardStats() to also catch the relevant transport/runtime failures (or a general failure type) and route them to the same $dashboardError banner path. Keep the auth/authz handling intact, but make sure unexpected HTTP/network/parse errors are converted into the friendly dashboard message instead of bubbling up.tests/Unit/EventSubscriber/UnauthorizedSubscriberTest.php (1)
82-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the non-XHR authorization branch as well.
The production branch at
UnauthorizedSubscriberLine 50 returns a plain 403 response, but this test only exercises the JSON/XHR path.Suggested additional test
public function testOnKernelExceptionWithAuthorizationException(): void { $authException = new AuthorizationException('No valid session key was provided as basic auth password.', 403); ... $this->assertEquals('No valid session key was provided as basic auth password.', $data['message']); } + + public function testOnKernelExceptionWithAuthorizationExceptionForBrowserRequest(): void + { + $authException = new AuthorizationException('Forbidden', 403); + + $request = $this->createMock(Request::class); + $request->method('isXmlHttpRequest')->willReturn(false); + + $kernel = $this->createMock(HttpKernelInterface::class); + $event = new ExceptionEvent( + $kernel, + $request, + HttpKernelInterface::MAIN_REQUEST, + $authException + ); + + $this->subscriber->onKernelException($event); + + $response = $event->getResponse(); + $this->assertNotNull($response); + $this->assertEquals(403, $response->getStatusCode()); + $this->assertEquals('Access denied.', $response->getContent()); + }🤖 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/UnauthorizedSubscriberTest.php` around lines 82 - 112, The current UnauthorizedSubscriberTest only covers the XHR/JSON authorization path in onKernelExceptionWithAuthorizationException, so add a separate assertion case for the non-XHR branch. Reuse AuthorizationException, ExceptionEvent, and UnauthorizedSubscriber::onKernelException with Request::isXmlHttpRequest returning false, then verify the response is a plain 403 response rather than a JsonResponse and that the status code is still 403.
🤖 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/settings/CreateAdminAttributeModal.vue`:
- Around line 42-51: The type selector in CreateAdminAttributeModal.vue is using
values that are not accepted by AdminAttributeDefinitionRequest.type. Update the
<select> options and the form default for form.type so they only use the API
enum values textline and hidden, and make sure the v-model binding and any
related initialization in the component stay consistent with those allowed
values.
In `@assets/vue/components/settings/EditAdminAttributeModal.vue`:
- Around line 42-51: The EditAdminAttributeModal select is using UI
labels/values that do not match the API enum, so existing values set by the form
watcher can render blank and be overwritten on save. Update the options in
EditAdminAttributeModal.vue to use the same enum values as the API, and make
sure the form.type binding in the edit flow continues to round-trip correctly
through the watcher and save handler.
In `@src/EventSubscriber/UnauthorizedSubscriber.php`:
- Around line 38-50: In UnauthorizedSubscriber::onKernelException, stop reusing
AuthorizationException::getMessage() for the user-facing 403 body; always return
a generic “Access denied.” message for both the JsonResponse and the plain
Response, while keeping any detailed upstream text out of the response. Update
the branch that handles $event->getRequest()->isXmlHttpRequest() and the
non-JSON Response path so they both use the same safe fallback, and adjust the
matching test assertion to expect the generic message instead of the exception
text.
---
Nitpick comments:
In `@assets/vue/components/campaigns/CampaignDirectory.vue`:
- Around line 739-808: The auth guard in the campaign statistics fetch is
missing the wrapped HTTP status shape used by the API client. Update the catch
block in fetchCampaignStatistics within CampaignDirectory.vue to treat
error?.response?.status === 403 the same as the existing AuthorizationException
checks, and then keep the fallback behavior of clearing statisticsByCampaignId
and hiding showStatistics. If you want to avoid losing the campaigns list on
non-auth stats failures, handle those errors locally in fetchCampaignStatistics
instead of rethrowing them so fetchCampaigns and Promise.all can still succeed.
In `@assets/vue/components/settings/CreateAdminAttributeModal.vue`:
- Around line 13-18: The close control in CreateAdminAttributeModal.vue is
currently exposed only as a glyph, so screen readers won’t know its purpose.
Update the button that emits the close event to include a clear accessible label
using an aria-label on the same button element. Keep the existing click behavior
unchanged and make sure the label describes that it closes the modal.
In `@src/Controller/DashboardController.php`:
- Around line 27-32: The dashboard stats fetch in DashboardController::show
currently catches only AuthenticationException and AuthorizationException, so
other client failures can still break the page. Broaden the exception handling
around StatisticsClient::getDashboardStats() and buildDashboardStats() to also
catch the relevant transport/runtime failures (or a general failure type) and
route them to the same $dashboardError banner path. Keep the auth/authz handling
intact, but make sure unexpected HTTP/network/parse errors are converted into
the friendly dashboard message instead of bubbling up.
In `@tests/Unit/EventSubscriber/UnauthorizedSubscriberTest.php`:
- Around line 82-112: The current UnauthorizedSubscriberTest only covers the
XHR/JSON authorization path in onKernelExceptionWithAuthorizationException, so
add a separate assertion case for the non-XHR branch. Reuse
AuthorizationException, ExceptionEvent, and
UnauthorizedSubscriber::onKernelException with Request::isXmlHttpRequest
returning false, then verify the response is a plain 403 response rather than a
JsonResponse and that the status code is still 403.
🪄 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: 7c6989a8-8151-42e9-870b-fdd833dc078a
📒 Files selected for processing (19)
assets/vue/api.jsassets/vue/components/campaigns/CampaignDirectory.vueassets/vue/components/settings/CreateAdminAttributeModal.vueassets/vue/components/settings/CreateAdminModal.vueassets/vue/components/settings/EditAdminAttributeModal.vueassets/vue/components/settings/EditAdminModal.vueassets/vue/components/settings/SettingsAdminAttributes.vueassets/vue/views/DashboardView.spec.jsassets/vue/views/DashboardView.vuesrc/Controller/AuthController.phpsrc/Controller/BaseController.phpsrc/Controller/DashboardController.phpsrc/EventSubscriber/AuthGateSubscriber.phpsrc/EventSubscriber/UnauthorizedSubscriber.phpsrc/Security/SessionAuthenticator.phpsrc/Trait/RedirectValidationTrait.phptemplates/spa.html.twigtests/Integration/Controller/DashboardControllerTest.phptests/Unit/EventSubscriber/UnauthorizedSubscriberTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- assets/vue/components/settings/EditAdminModal.vue
- assets/vue/components/settings/CreateAdminModal.vue
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
assets/vue/components/settings/EditSubscriberAttributeModal.vue (1)
100-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTiny nit: the option name input here has no placeholder, while the create modal uses
"Name"(its order input also has an"Order"placeholder this one lacks). Aligning them keeps the two modals consistent.🤖 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/settings/EditSubscriberAttributeModal.vue` at line 100, The option name input in the EditSubscriberAttributeModal is missing a placeholder, unlike the corresponding field in the create modal, so update the input in this component to use the same “Name” placeholder (and keep its sibling “Order” field consistent as well) by aligning the relevant template fields in EditSubscriberAttributeModal with the create-modal inputs.
🤖 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/settings/CreateSubscriberAttributeModal.vue`:
- Around line 150-152: The create modal is using the wrong API client:
`CreateSubscriberAttributeModal.vue` imports `adminAttributeClient` even though
this flow is for subscriber attributes. Update the modal to use
`subscriberAttributesClient` (matching `SettingsSubscriberAttributes.vue`) and
call its `createAttributeDefinition(...)` method so newly created attributes are
written to the same endpoint that the list and delete actions use. Confirm the
request payload still matches the subscriber-attribute client’s expected shape
and keep the surrounding `createAttributeDefinition` submit flow intact.
In `@assets/vue/components/settings/EditSubscriberAttributeModal.vue`:
- Around line 148-150: The edit modal is using the wrong API client, so updates
go through adminAttributeClient instead of the subscriber attributes client.
Update the EditSubscriberAttributeModal.vue flow to use the same
subscriberAttributesClient as the rest of the subscriber attribute actions,
especially the updateAttributeDefinition path, and make sure any related methods
in this component reference the corrected client consistently.
In `@assets/vue/components/settings/SettingsSubscriberAttributes.vue`:
- Around line 243-257: The current fetchAttributes implementation only loads the
first page of subscriber attributes because it calls getAttributeDefinitions()
without paging, so update it to use the existing fetchAllAttributeDefinitions
helper from assets/vue/api.js instead. Keep the same loading/error state
handling in fetchAttributes, but replace the single request with the paginated
helper so attributes.value is populated with the full complete list rather than
only result.items/result.data from the first page.
---
Nitpick comments:
In `@assets/vue/components/settings/EditSubscriberAttributeModal.vue`:
- Line 100: The option name input in the EditSubscriberAttributeModal is missing
a placeholder, unlike the corresponding field in the create modal, so update the
input in this component to use the same “Name” placeholder (and keep its sibling
“Order” field consistent as well) by aligning the relevant template fields in
EditSubscriberAttributeModal with the create-modal inputs.
🪄 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: d161459b-7263-44cb-9917-4e7eda08b9c1
📒 Files selected for processing (9)
assets/vue/components/settings/CreateAdminAttributeModal.vueassets/vue/components/settings/CreateSubscriberAttributeModal.vueassets/vue/components/settings/EditAdminAttributeModal.vueassets/vue/components/settings/EditSubscriberAttributeModal.vueassets/vue/components/settings/SettingsActionsPanel.vueassets/vue/components/settings/SettingsAdminAttributes.vueassets/vue/components/settings/SettingsSubscriberAttributes.vuesrc/EventSubscriber/UnauthorizedSubscriber.phptests/Unit/EventSubscriber/UnauthorizedSubscriberTest.php
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/Unit/EventSubscriber/UnauthorizedSubscriberTest.php
- assets/vue/components/settings/CreateAdminAttributeModal.vue
- assets/vue/components/settings/EditAdminAttributeModal.vue
- assets/vue/components/settings/SettingsActionsPanel.vue
- assets/vue/components/settings/SettingsAdminAttributes.vue
- src/EventSubscriber/UnauthorizedSubscriber.php
Comment on lines
+150
to
+152
| <script setup> | ||
| import { computed, reactive, ref, watch } from 'vue'; | ||
| import { adminAttributeClient } from '../../api'; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Wrong client — this writes to admin attributes, not subscriber attributes. 🐇
This is the subscriber-attribute create modal, but it imports and calls adminAttributeClient.createAttributeDefinition(...). The parent SettingsSubscriberAttributes.vue reads and deletes through subscriberAttributesClient, so created records land on a different endpoint than the ones being listed/deleted. Net effect: you create something, the list refreshes, and your new attribute is nowhere to be seen.
🔧 Proposed fix
-import { adminAttributeClient } from '../../api';
+import { subscriberAttributesClient } from '../../api';- await adminAttributeClient.createAttributeDefinition({
+ await subscriberAttributesClient.createAttributeDefinition({
...form,
options: JSON.parse(JSON.stringify(form.options)),
});Worth confirming subscriberAttributesClient exposes createAttributeDefinition with this payload shape.
#!/bin/bash
# Confirm SubscriberAttributesClient exposes create/update method names used here
fd -t f -e js . node_modules/@tatevikgr/rest-api-client 2>/dev/null | head
rg -nP 'class\s+SubscriberAttributesClient' -A40 $(fd . node_modules/@tatevikgr/rest-api-client -e js 2>/dev/null) 2>/dev/null
rg -nP '(create|update|delete|get)AttributeDefinition' -C2 $(fd . node_modules/@tatevikgr/rest-api-client -e js 2>/dev/null) 2>/dev/nullAlso applies to: 224-241
🤖 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/settings/CreateSubscriberAttributeModal.vue` around
lines 150 - 152, The create modal is using the wrong API client:
`CreateSubscriberAttributeModal.vue` imports `adminAttributeClient` even though
this flow is for subscriber attributes. Update the modal to use
`subscriberAttributesClient` (matching `SettingsSubscriberAttributes.vue`) and
call its `createAttributeDefinition(...)` method so newly created attributes are
written to the same endpoint that the list and delete actions use. Confirm the
request payload still matches the subscriber-attribute client’s expected shape
and keep the surrounding `createAttributeDefinition` submit flow intact.
Comment on lines
+148
to
+150
| <script setup> | ||
| import { computed, reactive, ref, watch } from 'vue'; | ||
| import { adminAttributeClient } from '../../api'; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Same client mix-up here — update hits adminAttributeClient.
Same root cause as the create modal: this edit modal updates via adminAttributeClient.updateAttributeDefinition(...), while the parent lists/deletes through subscriberAttributesClient. Edits will silently target the wrong resource.
🔧 Proposed fix
-import { adminAttributeClient } from '../../api';
+import { subscriberAttributesClient } from '../../api';- await adminAttributeClient.updateAttributeDefinition(
+ await subscriberAttributesClient.updateAttributeDefinition(
form.id,
JSON.parse(JSON.stringify(form))
);Also applies to: 224-241
🤖 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/settings/EditSubscriberAttributeModal.vue` around lines
148 - 150, The edit modal is using the wrong API client, so updates go through
adminAttributeClient instead of the subscriber attributes client. Update the
EditSubscriberAttributeModal.vue flow to use the same subscriberAttributesClient
as the rest of the subscriber attribute actions, especially the
updateAttributeDefinition path, and make sure any related methods in this
component reference the corrected client consistently.
Comment on lines
+243
to
+257
| const fetchAttributes = async () => { | ||
| isLoading.value = true | ||
| loadError.value = '' | ||
|
|
||
| try { | ||
| const result = await subscriberAttributesClient.getAttributeDefinitions() | ||
| attributes.value = result.items ?? result.data ?? [] | ||
| } catch (e) { | ||
| console.error(e) | ||
| loadError.value = e?.message || 'Failed to load attributes.' | ||
| attributes.value = [] | ||
| } finally { | ||
| isLoading.value = false | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Heads up: this only grabs the first page of attributes.
getAttributeDefinitions() is called with no cursor/limit, so anything past the first page silently won't show up. There's already a fetchAllAttributeDefinitions helper in assets/vue/api.js (lines 134-154) that handles the cursor pagination loop — worth reusing here so the list stays complete as definitions grow.
🤖 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/settings/SettingsSubscriberAttributes.vue` around lines
243 - 257, The current fetchAttributes implementation only loads the first page
of subscriber attributes because it calls getAttributeDefinitions() without
paging, so update it to use the existing fetchAllAttributeDefinitions helper
from assets/vue/api.js instead. Keep the same loading/error state handling in
fetchAttributes, but replace the single request with the paginated helper so
attributes.value is populated with the full complete list rather than only
result.items/result.data from the first page.
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
/settings) with tabbed views for Configs, Admins, Admin Attributes, and Subscriber Attributes.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!