Conversation
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR adds an Analytics page end to end, including a Symfony controller, Vue route, dashboard view, and integration/unit tests. It also updates Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AnalyticsController
participant Session
participant AnalyticsView
participant statisticsClient
User->>AnalyticsController: GET /analytics/
AnalyticsController->>Session: read auth_token
AnalyticsController-->>User: render SPA shell
User->>AnalyticsView: mount
AnalyticsView->>statisticsClient: load analytics datasets
statisticsClient-->>AnalyticsView: statistics responses
AnalyticsView-->>User: render dashboard
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 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/views/AnalyticsView.vue`:
- Around line 71-125: The empty-state sections in AnalyticsView.vue are
rendering before loadAnalytics() finishes, which causes a misleading “no data”
flash on first paint. Update the loading logic in AnalyticsView and the affected
empty-state blocks to stay hidden until the initial fetch completes, either by
introducing a hasLoaded flag or by guarding all “No ... found” branches with
!isLoading. Make sure the fix is applied consistently across the
domainConfirmation view and the other matching empty states so the template only
shows “no data” after the data request has actually finished.
- Around line 306-320: The formatDate helper in AnalyticsView.vue currently
passes date-only strings straight into new Date, which can shift the displayed
day in some time zones. Update formatDate to detect YYYY-MM-DD inputs and
normalize them before formatting, while keeping the existing Unknown date
fallback and Intl.DateTimeFormat output.
🪄 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: 137c0e28-face-46b0-ab9a-c3fa52df4efc
📒 Files selected for processing (6)
assets/router/index.jsassets/vue/api.jsassets/vue/views/AnalyticsView.spec.jsassets/vue/views/AnalyticsView.vuesrc/Controller/AnalyticsController.phptests/Integration/Controller/AnalyticsControllerTest.php
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Unit/assets/vue/views/AnalyticsView.spec.js (1)
19-21: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winMock the same
apimodule the component imports. Intests/Unit/assets/vue/views/AnalyticsView.spec.js:19,vi.mock('../api', ...)resolves totests/Unit/assets/vue/api, whileassets/vue/views/AnalyticsView.vueimports../apifrom the source tree. Point the mock at the component’s resolved path so the test doesn’t fall through to the real client.🤖 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/assets/vue/views/AnalyticsView.spec.js` around lines 19 - 21, The mock in AnalyticsView.spec.js is targeting the wrong module path, so AnalyticsView.vue still resolves its own ../api import to the real client. Update the vi.mock call to match the component’s actual resolved api module path used by AnalyticsView.vue, and keep the statisticsClient stub there so the test intercepts the same import the component loads.
♻️ Duplicate comments (2)
tests/Unit/assets/vue/components/settings/SettingsConfigs.spec.js (1)
12-18: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame
vi.mockrelative-path mismatch as in the other settings modal/panel specs.
'../../api'resolves relative to this spec's own directory, landing ontests/Unit/assets/vue/apiinstead of the realassets/vue/api.jsimported bySettingsConfigs.vue, soconfigClientcalls likely bypass the mock.🔧 Proposed fix
-vi.mock('../../api', () => ({ +vi.mock('../../../../../../assets/vue/api', () => ({ default: {}, configClient: { getConfigs: getConfigsMock, update: updateMock, }, }))🤖 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/assets/vue/components/settings/SettingsConfigs.spec.js` around lines 12 - 18, The SettingsConfigs spec is mocking the wrong relative module path, so the `configClient` mock may not intercept the import used by `SettingsConfigs.vue`. Update the `vi.mock` call in `SettingsConfigs.spec.js` to target the same resolved `assets/vue/api` module path that the component imports, matching the pattern used in the other settings modal/panel specs, so `getConfigs` and `update` are properly mocked.tests/Unit/assets/vue/components/settings/SettingsSubscriberAttributes.spec.js (1)
13-42: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame
vi.mockrelative-path mismatch, affecting all 4 mocks in this file.All of
'../../api'(Line 13),'../base/BaseIcon.vue'(Line 20),'./CreateSubscriberAttributeModal.vue'(Line 26), and'./EditSubscriberAttributeModal.vue'(Line 35) are resolved relative to this spec file's directory, not relative toSettingsSubscriberAttributes.vue's actual location — so none of them likely intercept the real imports the component makes.🔧 Proposed fix
-vi.mock('../../api', () => ({ +vi.mock('../../../../../../assets/vue/api', () => ({ subscriberAttributesClient: { getAttributeDefinitions: getAttributeDefinitionsMock, deleteAttributeDefinition: deleteAttributeDefinitionMock, }, })) -vi.mock('../base/BaseIcon.vue', () => ({ +vi.mock('../../../../../../assets/vue/components/base/BaseIcon.vue', () => ({ default: { template: '<span />', }, })) -vi.mock('./CreateSubscriberAttributeModal.vue', () => ({ +vi.mock('../../../../../../assets/vue/components/settings/CreateSubscriberAttributeModal.vue', () => ({ default: defineComponent({ name: 'CreateSubscriberAttributeModal', props: ['isOpen'], emits: ['close', 'created'], template: '<div />', }), })) -vi.mock('./EditSubscriberAttributeModal.vue', () => ({ +vi.mock('../../../../../../assets/vue/components/settings/EditSubscriberAttributeModal.vue', () => ({ default: defineComponent({ name: 'EditSubscriberAttributeModal', props: ['isOpen', 'attribute'], emits: ['close', 'updated'], template: '<div />', }), }))🤖 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/assets/vue/components/settings/SettingsSubscriberAttributes.spec.js` around lines 13 - 42, The four vi.mock calls in SettingsSubscriberAttributes.spec.js are using paths relative to the spec file instead of matching the imports used by SettingsSubscriberAttributes.vue, so they won’t stub the real dependencies. Update the mocked module specifiers for subscriberAttributesClient, BaseIcon, CreateSubscriberAttributeModal, and EditSubscriberAttributeModal to the exact paths that SettingsSubscriberAttributes.vue imports, using the existing mock names to locate each entry.
🧹 Nitpick comments (3)
vitest.config.mjs (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the stale
assets/vue/**/*.spec.jsglob. It matches nothing now, sotests/Unit/assets/vue/**/*.spec.jsis enough.🤖 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 `@vitest.config.mjs` at line 9, The Vitest include list still contains the stale assets/vue/**/*.spec.js glob, which no longer matches any files. Update the include configuration in vitest.config.mjs to keep only the valid tests/Unit/assets/vue/**/*.spec.js pattern so the test discovery remains accurate.tests/Unit/assets/vue/components/settings/SettingsAdminAttributes.spec.js (1)
126-136: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest asserts nothing.
reloads attributes after createonly emits'created'and never assertsgetAttributeDefinitionsMockwas called again — it will pass regardless of whether the reload actually happens, defeating the purpose of the test.🔧 Suggested fix
it('reloads attributes after create', async () => { const wrapper = mountComponent() await flushPromises() getAttributeDefinitionsMock.mockClear() const createModal = wrapper.findComponent(CreateStub) expect(createModal.exists()).toBe(true) await createModal.vm.$emit('created') + await flushPromises() + + expect(getAttributeDefinitionsMock).toHaveBeenCalled() })🤖 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/assets/vue/components/settings/SettingsAdminAttributes.spec.js` around lines 126 - 136, The reload-after-create test is not verifying any behavior because it only emits created and never checks that the refresh happens. Update the SettingsAdminAttributes.spec.js test around reloads attributes after create to assert the expected reload behavior by checking getAttributeDefinitionsMock is called again after the CreateStub emits created, so the test fails if the refresh does not occur.tests/Unit/assets/vue/components/settings/CreateSubscriberAttributeModal.spec.js (1)
96-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFragile selector for the "remove" button.
find(button => !button.text().trim())picks the first button with empty text, which is brittle if markup adds an icon-only close button elsewhere in the modal. Low priority given "Chill" review mode, but worth adata-testiddown the line.🤖 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/assets/vue/components/settings/CreateSubscriberAttributeModal.spec.js` around lines 96 - 116, The remove-button lookup in CreateSubscriberAttributeModal.spec.js is brittle because it uses an empty-text button search that can accidentally match other icon-only buttons. Update the test around mountComponent and the “adds and removes options” case to target a stable identifier instead, such as a data-testid or a dedicated selector on the remove control, and use that for removeButton so the assertion only clicks the intended button.
🤖 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 `@tests/Unit/assets/vue/components/bounces/BouncePer.spec.js`:
- Around line 3-4: The vi.mock path in BouncePer.spec.js is targeting the wrong
module, so it does not intercept the api import used by BouncePer.vue. Update
the mock to match the component’s actual relative import for bouncesClient from
assets/vue/api, and keep the test import of BouncePer aligned so the mocked
module is the one exercised by the component under test.
In `@tests/Unit/assets/vue/components/settings/CreateAdminAttributeModal.spec.js`:
- Around line 10-14: The vi.mock target in CreateAdminAttributeModal.spec.js is
pointing at the wrong api module, so the component’s imported
adminAttributeClient is never replaced. Update the mock path to match the same
assets/vue/api module that CreateAdminAttributeModal.vue imports, and keep the
createAttributeDefinition stub wired through adminAttributeClient so the spec
uses the mocked client.
In `@tests/Unit/assets/vue/components/settings/CreateAdminModal.spec.js`:
- Around line 24-34: The mocks in CreateAdminModal.spec.js are targeting paths
that resolve relative to the test file instead of the modules imported by
CreateAdminModal.vue, so they never intercept the real dependencies. Update the
vi.mock calls to use the same module IDs that CreateAdminModal.vue imports for
the API client and BaseIcon component, and verify the mock names still match the
references used in the test setup.
In
`@tests/Unit/assets/vue/components/settings/CreateSubscriberAttributeModal.spec.js`:
- Around line 10-20: Fix the Vitest mocks in
CreateSubscriberAttributeModal.spec.js so they intercept the same import paths
used by the component. Update the vi.mock calls for the API module and
BaseIcon.vue to match the component’s assets/vue/api.js and
assets/vue/components/base/BaseIcon.vue imports, ensuring
subscriberAttributesClient.createAttributeDefinition is actually mocked when the
modal tests run.
In `@tests/Unit/assets/vue/components/settings/EditAdminModal.spec.js`:
- Around line 24-34: The test mocks in EditAdminModal.spec.js are using module
ids that do not match the imports in EditAdminModal.vue, so the real adminClient
and BaseIcon dependencies are still being loaded. Update the vi.mock targets to
match the exact import paths used by EditAdminModal (for adminClient) and the
BaseIcon component so the update tests fully stub those dependencies.
In
`@tests/Unit/assets/vue/components/settings/EditSubscriberAttributeModal.spec.js`:
- Around line 10-20: The mocks in EditSubscriberAttributeModal.spec.js are
targeting the wrong module ids, so they do not intercept the imports used by
EditSubscriberAttributeModal.vue. Update the vi.mock calls for the api client
and BaseIcon.vue to point at the same source module paths that the component
imports, using the existing identifiers subscriberAttributesClient and
updateAttributeDefinitionMock so the component receives the intended stubs.
In `@tests/Unit/assets/vue/components/settings/SettingsAdminAttributes.spec.js`:
- Around line 13-24: The mocks in SettingsAdminAttributes.spec.js are resolving
from the test file location instead of the component’s import paths, so they do
not intercept the imports used by SettingsAdminAttributes.vue. Update the
vi.mock targets for the api client and BaseIcon.vue to match the component’s
actual relative imports (or switch both to a shared alias) so
adminAttributeClient and BaseIcon are properly mocked during the component test.
In `@tests/Unit/assets/vue/components/settings/SettingsAdmins.spec.js`:
- Around line 13-42: Update the mocks in SettingsAdmins.spec.js so they match
the import paths used by SettingsAdmins.vue rather than resolving from the spec
file; adjust the vi.mock targets for ../../api, ../base/BaseIcon.vue,
CreateAdminModal.vue, and EditAdminModal.vue to the component-relative asset
paths so the component under test actually receives the mocked modules.
In `@tests/Unit/assets/vue/components/subscribers/SubscriberModal.spec.js`:
- Around line 5-6: The test mock for subscribersClient is targeting a different
resolved module than the one SubscriberModal actually imports, so it will not
intercept the real dependency. Update the vi.mock setup in
SubscriberModal.spec.js to mock the same assets/vue/api module path used by
SubscriberModal, and keep the subscribersClient import aligned with that same
module resolution so the component under test receives the mocked client.
---
Outside diff comments:
In `@tests/Unit/assets/vue/views/AnalyticsView.spec.js`:
- Around line 19-21: The mock in AnalyticsView.spec.js is targeting the wrong
module path, so AnalyticsView.vue still resolves its own ../api import to the
real client. Update the vi.mock call to match the component’s actual resolved
api module path used by AnalyticsView.vue, and keep the statisticsClient stub
there so the test intercepts the same import the component loads.
---
Duplicate comments:
In `@tests/Unit/assets/vue/components/settings/SettingsConfigs.spec.js`:
- Around line 12-18: The SettingsConfigs spec is mocking the wrong relative
module path, so the `configClient` mock may not intercept the import used by
`SettingsConfigs.vue`. Update the `vi.mock` call in `SettingsConfigs.spec.js` to
target the same resolved `assets/vue/api` module path that the component
imports, matching the pattern used in the other settings modal/panel specs, so
`getConfigs` and `update` are properly mocked.
In
`@tests/Unit/assets/vue/components/settings/SettingsSubscriberAttributes.spec.js`:
- Around line 13-42: The four vi.mock calls in
SettingsSubscriberAttributes.spec.js are using paths relative to the spec file
instead of matching the imports used by SettingsSubscriberAttributes.vue, so
they won’t stub the real dependencies. Update the mocked module specifiers for
subscriberAttributesClient, BaseIcon, CreateSubscriberAttributeModal, and
EditSubscriberAttributeModal to the exact paths that
SettingsSubscriberAttributes.vue imports, using the existing mock names to
locate each entry.
---
Nitpick comments:
In
`@tests/Unit/assets/vue/components/settings/CreateSubscriberAttributeModal.spec.js`:
- Around line 96-116: The remove-button lookup in
CreateSubscriberAttributeModal.spec.js is brittle because it uses an empty-text
button search that can accidentally match other icon-only buttons. Update the
test around mountComponent and the “adds and removes options” case to target a
stable identifier instead, such as a data-testid or a dedicated selector on the
remove control, and use that for removeButton so the assertion only clicks the
intended button.
In `@tests/Unit/assets/vue/components/settings/SettingsAdminAttributes.spec.js`:
- Around line 126-136: The reload-after-create test is not verifying any
behavior because it only emits created and never checks that the refresh
happens. Update the SettingsAdminAttributes.spec.js test around reloads
attributes after create to assert the expected reload behavior by checking
getAttributeDefinitionsMock is called again after the CreateStub emits created,
so the test fails if the refresh does not occur.
In `@vitest.config.mjs`:
- Line 9: The Vitest include list still contains the stale
assets/vue/**/*.spec.js glob, which no longer matches any files. Update the
include configuration in vitest.config.mjs to keep only the valid
tests/Unit/assets/vue/**/*.spec.js pattern so the test discovery remains
accurate.
🪄 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: 01cd48f1-4350-4439-a850-7a50220d3540
📒 Files selected for processing (54)
.github/workflows/ci.yml.husky/pre-commitassets/vue/components/settings/EditAdminModal.vuetests/Unit/assets/vue/components/base/BaseBadge.spec.jstests/Unit/assets/vue/components/base/BaseButton.spec.jstests/Unit/assets/vue/components/base/BaseCard.spec.jstests/Unit/assets/vue/components/base/BaseIcon.spec.jstests/Unit/assets/vue/components/base/BaseProgressBar.spec.jstests/Unit/assets/vue/components/base/CkEditorField.spec.jstests/Unit/assets/vue/components/bounces/BounceOverview.spec.jstests/Unit/assets/vue/components/bounces/BouncePer.spec.jstests/Unit/assets/vue/components/bounces/BounceRules.spec.jstests/Unit/assets/vue/components/bounces/BouncesActionsPanel.spec.jstests/Unit/assets/vue/components/campaigns/CampaignDirectory.spec.jstests/Unit/assets/vue/components/campaigns/ViewCampaignModal.spec.jstests/Unit/assets/vue/components/dashboard/CampaignsTable.spec.jstests/Unit/assets/vue/components/dashboard/KpiCard.spec.jstests/Unit/assets/vue/components/dashboard/KpiGrid.spec.jstests/Unit/assets/vue/components/dashboard/PerformanceChartCard.spec.jstests/Unit/assets/vue/components/dashboard/QuickActionsCard.spec.jstests/Unit/assets/vue/components/dashboard/RecentCampaignsCard.spec.jstests/Unit/assets/vue/components/lists/AddSubscribersModal.spec.jstests/Unit/assets/vue/components/lists/CreateListModal.spec.jstests/Unit/assets/vue/components/lists/EditListModal.spec.jstests/Unit/assets/vue/components/lists/ListDirectory.spec.jstests/Unit/assets/vue/components/lists/ListSubscribersExportPanel.spec.jstests/Unit/assets/vue/components/public-pages/PublicPageEditor.spec.jstests/Unit/assets/vue/components/public-pages/PublicPagesDirectory.spec.jstests/Unit/assets/vue/components/settings/CreateAdminAttributeModal.spec.jstests/Unit/assets/vue/components/settings/CreateAdminModal.spec.jstests/Unit/assets/vue/components/settings/CreateSubscriberAttributeModal.spec.jstests/Unit/assets/vue/components/settings/EditAdminAttributeModal.spec.jstests/Unit/assets/vue/components/settings/EditAdminModal.spec.jstests/Unit/assets/vue/components/settings/EditSubscriberAttributeModal.spec.jstests/Unit/assets/vue/components/settings/SettingsActionsPanel.spec.jstests/Unit/assets/vue/components/settings/SettingsAdminAttributes.spec.jstests/Unit/assets/vue/components/settings/SettingsAdmins.spec.jstests/Unit/assets/vue/components/settings/SettingsConfigs.spec.jstests/Unit/assets/vue/components/settings/SettingsSubscriberAttributes.spec.jstests/Unit/assets/vue/components/sidebar/AppSidebar.spec.jstests/Unit/assets/vue/components/sidebar/SidebarLogo.spec.jstests/Unit/assets/vue/components/sidebar/SidebarNavItem.spec.jstests/Unit/assets/vue/components/sidebar/SidebarNavSection.spec.jstests/Unit/assets/vue/components/subscribers/ImportResult.spec.jstests/Unit/assets/vue/components/subscribers/SubscriberDirectory.spec.jstests/Unit/assets/vue/components/subscribers/SubscriberFilters.spec.jstests/Unit/assets/vue/components/subscribers/SubscriberModal.spec.jstests/Unit/assets/vue/components/subscribers/SubscriberTable.spec.jstests/Unit/assets/vue/components/templates/TemplateLibrary.spec.jstests/Unit/assets/vue/layouts/AdminLayout.spec.jstests/Unit/assets/vue/views/AnalyticsView.spec.jstests/Unit/assets/vue/views/BouncesView.spec.jstests/Unit/assets/vue/views/DashboardView.spec.jsvitest.config.mjs
✅ Files skipped from review due to trivial changes (21)
- tests/Unit/assets/vue/components/base/BaseButton.spec.js
- tests/Unit/assets/vue/components/base/BaseCard.spec.js
- .husky/pre-commit
- tests/Unit/assets/vue/views/BouncesView.spec.js
- tests/Unit/assets/vue/components/public-pages/PublicPagesDirectory.spec.js
- tests/Unit/assets/vue/components/base/BaseProgressBar.spec.js
- tests/Unit/assets/vue/components/lists/EditListModal.spec.js
- tests/Unit/assets/vue/components/campaigns/ViewCampaignModal.spec.js
- tests/Unit/assets/vue/layouts/AdminLayout.spec.js
- tests/Unit/assets/vue/components/subscribers/ImportResult.spec.js
- tests/Unit/assets/vue/components/public-pages/PublicPageEditor.spec.js
- tests/Unit/assets/vue/components/subscribers/SubscriberTable.spec.js
- tests/Unit/assets/vue/components/base/BaseIcon.spec.js
- tests/Unit/assets/vue/components/sidebar/AppSidebar.spec.js
- tests/Unit/assets/vue/components/templates/TemplateLibrary.spec.js
- tests/Unit/assets/vue/components/lists/ListDirectory.spec.js
- tests/Unit/assets/vue/components/subscribers/SubscriberFilters.spec.js
- tests/Unit/assets/vue/components/base/BaseBadge.spec.js
- tests/Unit/assets/vue/components/settings/EditAdminAttributeModal.spec.js
- tests/Unit/assets/vue/components/sidebar/SidebarNavItem.spec.js
- tests/Unit/assets/vue/components/settings/SettingsActionsPanel.spec.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Unit/assets/vue/views/DashboardView.spec.js (1)
40-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOnly the error path is covered — worth a happy-path test too.
This suite nails the error-banner scenario, but there's no test asserting
DashboardViewrenders correctly with validdata-dashboard-stats(e.g., verifyingKpiGrid/PerformanceChartCard/etc. mount without the alert). Since those are already mocked, a quick seconditblock would round out coverage cheaply.🧪 Example additional test
it('renders dashboard blocks when stats are provided without an error', async () => { document.body.innerHTML = ` <div id="vue-app" data-dashboard-stats='{"foo":"bar"}' ></div> ` vi.resetModules() const { default: DashboardView } = await import('../../../../../assets/vue/views/DashboardView.vue') const wrapper = mount(DashboardView) expect(wrapper.find('[role="alert"]').exists()).toBe(false) expect(wrapper.findAll('.dashboard-block').length).toBeGreaterThan(0) })🤖 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/assets/vue/views/DashboardView.spec.js` around lines 40 - 63, Add a happy-path test for DashboardView alongside the existing error-banner case: in DashboardView.spec.js, mount the component with valid data-dashboard-stats and no data-dashboard-error, then assert the alert is absent and the dashboard content renders (for example, mocked blocks like KpiGrid/PerformanceChartCard appear). Use the existing DashboardView import/mount pattern so the new test matches the current suite setup.tests/Unit/assets/vue/views/CampaignEditView.spec.js (1)
391-440: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
beforeEachmount in "Format Tab" block.The
beforeEachat Line 392-400 mounts a component and clicks to step 2, but the wrapper is discarded — every test below re-mounts and re-clicks independently. ThisbeforeEachdoes nothing but waste cycles on every test run.🧹 Suggested cleanup
describe('Format Tab (Step 2)', () => { - beforeEach(async () => { - const wrapper = mountComponent() - await flushPromises() - - const stepButtons = wrapper.findAll('button') - const step2Button = stepButtons.find((btn) => btn.text().includes('2')) - await step2Button.trigger('click') - await wrapper.vm.$nextTick() - }) - it('displays template dropdown', async () => {🤖 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/assets/vue/views/CampaignEditView.spec.js` around lines 391 - 440, The Format Tab (Step 2) suite has a redundant beforeEach that mounts the component and switches to step 2, but each test in CampaignEditView.spec.js already does its own mountComponent/flushPromises/click flow. Remove the unused beforeEach from the Format Tab describe block and keep the setup only in the individual tests that actually need the wrapper.
🤖 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 `@tests/Unit/assets/vue/views/TemplateEditorView.spec.js`:
- Around line 6-12: The module-level mockRoute object in
TemplateEditorView.spec.js is being mutated by one test and not reset, which can
leak state into other tests. Update the shared test setup so the original
edit-mode shape of mockRoute is restored before each test, alongside
vi.restoreAllMocks(), and avoid relying on test order; use the mockRoute object
and the template-edit/template-create test cases to locate the affected setup
and mutation points.
---
Nitpick comments:
In `@tests/Unit/assets/vue/views/CampaignEditView.spec.js`:
- Around line 391-440: The Format Tab (Step 2) suite has a redundant beforeEach
that mounts the component and switches to step 2, but each test in
CampaignEditView.spec.js already does its own mountComponent/flushPromises/click
flow. Remove the unused beforeEach from the Format Tab describe block and keep
the setup only in the individual tests that actually need the wrapper.
In `@tests/Unit/assets/vue/views/DashboardView.spec.js`:
- Around line 40-63: Add a happy-path test for DashboardView alongside the
existing error-banner case: in DashboardView.spec.js, mount the component with
valid data-dashboard-stats and no data-dashboard-error, then assert the alert is
absent and the dashboard content renders (for example, mocked blocks like
KpiGrid/PerformanceChartCard appear). Use the existing DashboardView
import/mount pattern so the new test matches the current suite setup.
🪄 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: 4920ac2a-b596-46d4-af09-d10776d008d8
📒 Files selected for processing (65)
.github/workflows/ci.yml.husky/pre-commitassets/vue/components/dashboard/KpiCard.vueassets/vue/views/DashboardView.spec.jstests/Unit/assets/vue/components/base/BaseBadge.spec.jstests/Unit/assets/vue/components/base/BaseButton.spec.jstests/Unit/assets/vue/components/base/BaseCard.spec.jstests/Unit/assets/vue/components/base/BaseIcon.spec.jstests/Unit/assets/vue/components/base/BaseProgressBar.spec.jstests/Unit/assets/vue/components/base/CkEditorField.spec.jstests/Unit/assets/vue/components/bounces/BounceOverview.spec.jstests/Unit/assets/vue/components/bounces/BouncePer.spec.jstests/Unit/assets/vue/components/bounces/BounceRules.spec.jstests/Unit/assets/vue/components/bounces/BouncesActionsPanel.spec.jstests/Unit/assets/vue/components/campaigns/CampaignDirectory.spec.jstests/Unit/assets/vue/components/campaigns/ViewCampaignModal.spec.jstests/Unit/assets/vue/components/dashboard/CampaignsTable.spec.jstests/Unit/assets/vue/components/dashboard/KpiCard.spec.jstests/Unit/assets/vue/components/dashboard/KpiGrid.spec.jstests/Unit/assets/vue/components/dashboard/PerformanceChartCard.spec.jstests/Unit/assets/vue/components/dashboard/QuickActionsCard.spec.jstests/Unit/assets/vue/components/dashboard/RecentCampaignsCard.spec.jstests/Unit/assets/vue/components/lists/AddSubscribersModal.spec.jstests/Unit/assets/vue/components/lists/CreateListModal.spec.jstests/Unit/assets/vue/components/lists/EditListModal.spec.jstests/Unit/assets/vue/components/lists/ListDirectory.spec.jstests/Unit/assets/vue/components/lists/ListSubscribersExportPanel.spec.jstests/Unit/assets/vue/components/public-pages/PublicPageEditor.spec.jstests/Unit/assets/vue/components/public-pages/PublicPagesDirectory.spec.jstests/Unit/assets/vue/components/settings/CreateAdminAttributeModal.spec.jstests/Unit/assets/vue/components/settings/CreateAdminModal.spec.jstests/Unit/assets/vue/components/settings/CreateSubscriberAttributeModal.spec.jstests/Unit/assets/vue/components/settings/EditAdminAttributeModal.spec.jstests/Unit/assets/vue/components/settings/EditAdminModal.spec.jstests/Unit/assets/vue/components/settings/EditSubscriberAttributeModal.spec.jstests/Unit/assets/vue/components/settings/SettingsActionsPanel.spec.jstests/Unit/assets/vue/components/settings/SettingsAdminAttributes.spec.jstests/Unit/assets/vue/components/settings/SettingsAdmins.spec.jstests/Unit/assets/vue/components/settings/SettingsConfigs.spec.jstests/Unit/assets/vue/components/settings/SettingsSubscriberAttributes.spec.jstests/Unit/assets/vue/components/sidebar/AppSidebar.spec.jstests/Unit/assets/vue/components/sidebar/SidebarLogo.spec.jstests/Unit/assets/vue/components/sidebar/SidebarNavItem.spec.jstests/Unit/assets/vue/components/sidebar/SidebarNavSection.spec.jstests/Unit/assets/vue/components/subscribers/ImportResult.spec.jstests/Unit/assets/vue/components/subscribers/SubscriberDirectory.spec.jstests/Unit/assets/vue/components/subscribers/SubscriberFilters.spec.jstests/Unit/assets/vue/components/subscribers/SubscriberModal.spec.jstests/Unit/assets/vue/components/subscribers/SubscriberTable.spec.jstests/Unit/assets/vue/components/templates/TemplateLibrary.spec.jstests/Unit/assets/vue/layouts/AdminLayout.spec.jstests/Unit/assets/vue/views/AnalyticsView.spec.jstests/Unit/assets/vue/views/BouncesView.spec.jstests/Unit/assets/vue/views/CampaignEditView.spec.jstests/Unit/assets/vue/views/CampaignsView.spec.jstests/Unit/assets/vue/views/DashboardView.spec.jstests/Unit/assets/vue/views/ListSubscribersView.spec.jstests/Unit/assets/vue/views/ListsView.spec.jstests/Unit/assets/vue/views/PublicPageEditView.spec.jstests/Unit/assets/vue/views/PublicPagesView.spec.jstests/Unit/assets/vue/views/SettingsView.spec.jstests/Unit/assets/vue/views/SubscribersView.spec.jstests/Unit/assets/vue/views/TemplateEditorView.spec.jstests/Unit/assets/vue/views/TemplatesView.spec.jsvitest.config.mjs
💤 Files with no reviewable changes (1)
- assets/vue/views/DashboardView.spec.js
✅ Files skipped from review due to trivial changes (18)
- tests/Unit/assets/vue/components/subscribers/SubscriberTable.spec.js
- tests/Unit/assets/vue/views/TemplatesView.spec.js
- tests/Unit/assets/vue/components/dashboard/KpiCard.spec.js
- tests/Unit/assets/vue/components/base/CkEditorField.spec.js
- tests/Unit/assets/vue/components/base/BaseIcon.spec.js
- tests/Unit/assets/vue/components/public-pages/PublicPagesDirectory.spec.js
- tests/Unit/assets/vue/components/bounces/BouncePer.spec.js
- tests/Unit/assets/vue/components/dashboard/RecentCampaignsCard.spec.js
- tests/Unit/assets/vue/components/campaigns/ViewCampaignModal.spec.js
- tests/Unit/assets/vue/components/subscribers/SubscriberFilters.spec.js
- tests/Unit/assets/vue/components/lists/CreateListModal.spec.js
- tests/Unit/assets/vue/components/lists/AddSubscribersModal.spec.js
- .husky/pre-commit
- tests/Unit/assets/vue/components/sidebar/SidebarNavSection.spec.js
- tests/Unit/assets/vue/components/dashboard/CampaignsTable.spec.js
- tests/Unit/assets/vue/components/bounces/BounceOverview.spec.js
- tests/Unit/assets/vue/components/sidebar/SidebarNavItem.spec.js
- tests/Unit/assets/vue/components/dashboard/PerformanceChartCard.spec.js
🚧 Files skipped from review as they are similar to previous changes (33)
- tests/Unit/assets/vue/components/base/BaseCard.spec.js
- tests/Unit/assets/vue/components/base/BaseBadge.spec.js
- .github/workflows/ci.yml
- tests/Unit/assets/vue/views/BouncesView.spec.js
- tests/Unit/assets/vue/components/sidebar/AppSidebar.spec.js
- tests/Unit/assets/vue/components/sidebar/SidebarLogo.spec.js
- tests/Unit/assets/vue/components/base/BaseButton.spec.js
- tests/Unit/assets/vue/components/templates/TemplateLibrary.spec.js
- tests/Unit/assets/vue/components/dashboard/KpiGrid.spec.js
- tests/Unit/assets/vue/components/campaigns/CampaignDirectory.spec.js
- tests/Unit/assets/vue/layouts/AdminLayout.spec.js
- tests/Unit/assets/vue/components/dashboard/QuickActionsCard.spec.js
- tests/Unit/assets/vue/components/base/BaseProgressBar.spec.js
- tests/Unit/assets/vue/components/subscribers/ImportResult.spec.js
- tests/Unit/assets/vue/components/lists/ListSubscribersExportPanel.spec.js
- tests/Unit/assets/vue/components/lists/EditListModal.spec.js
- tests/Unit/assets/vue/components/lists/ListDirectory.spec.js
- tests/Unit/assets/vue/components/subscribers/SubscriberModal.spec.js
- tests/Unit/assets/vue/components/bounces/BounceRules.spec.js
- tests/Unit/assets/vue/components/subscribers/SubscriberDirectory.spec.js
- tests/Unit/assets/vue/components/settings/EditAdminModal.spec.js
- tests/Unit/assets/vue/views/AnalyticsView.spec.js
- tests/Unit/assets/vue/components/settings/CreateAdminAttributeModal.spec.js
- tests/Unit/assets/vue/components/settings/SettingsSubscriberAttributes.spec.js
- tests/Unit/assets/vue/components/settings/EditAdminAttributeModal.spec.js
- tests/Unit/assets/vue/components/settings/SettingsConfigs.spec.js
- vitest.config.mjs
- tests/Unit/assets/vue/components/settings/CreateSubscriberAttributeModal.spec.js
- tests/Unit/assets/vue/components/settings/SettingsAdminAttributes.spec.js
- tests/Unit/assets/vue/components/settings/SettingsAdmins.spec.js
- tests/Unit/assets/vue/components/settings/EditSubscriberAttributeModal.spec.js
- tests/Unit/assets/vue/components/settings/SettingsActionsPanel.spec.js
- tests/Unit/assets/vue/components/settings/CreateAdminModal.spec.js
Comment on lines
+6
to
+12
| const mockRoute = { | ||
| name: 'template-edit', | ||
| params: { | ||
| templateId: '1', | ||
| }, | ||
| query: {}, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Shared mockRoute mutated without reset — test pollution risk.
mockRoute is a module-level object mutated in-place by the last test (mockRoute.name = 'template-create'; mockRoute.params.templateId = ''), but beforeEach only calls vi.restoreAllMocks(), never resetting mockRoute back to its original edit-mode shape. This works today only because the mutating test happens to run last; reordering tests, adding a new test afterward, or running with --sequence.shuffle will leak create-mode state into unrelated tests.
🔧 Suggested fix
+const defaultRoute = {
+ name: 'template-edit',
+ params: { templateId: '1' },
+ query: {},
+}
+
-const mockRoute = {
- name: 'template-edit',
- params: {
- templateId: '1',
- },
- query: {},
-}
+const mockRoute = { ...defaultRoute }
describe('TemplateEditView', () => {
beforeEach(() => {
vi.restoreAllMocks()
+ Object.assign(mockRoute, JSON.parse(JSON.stringify(defaultRoute)))Also applies to: 100-112
🤖 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/assets/vue/views/TemplateEditorView.spec.js` around lines 6 - 12,
The module-level mockRoute object in TemplateEditorView.spec.js is being mutated
by one test and not reset, which can leak state into other tests. Update the
shared test setup so the original edit-mode shape of mockRoute is restored
before each test, alongside vi.restoreAllMocks(), and avoid relying on test
order; use the mockRoute object and the template-edit/template-create test cases
to locate the affected setup and mutation points.
TatevikGr
added a commit
that referenced
this pull request
Jul 7, 2026
* Feat: statistics * Feat: statistics * After review 0 * Add tests * Add tests 1 * Fix workflow file, add more tests --------- Co-authored-by: Tatevik <tatevikg1@gmail.com>
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
/analyticswith campaign/domain metrics, interactive charts, and performance tables.Bug Fixes
Tests
Chores
Thanks for contributing to phpList!