Conversation
* Refactor: env * fix test * app.api_base_url * Remove api_base_url from controller render methods and add it as a global variable in twig configuration * Remove api_token from controller render methods and update spa.html.twig to use app.session for auth_token * Update API base URL configuration in templates and JavaScript --------- Co-authored-by: Tatevik <tatevikg1@gmail.com>
|
Warning Review limit reached
Next review available in: 22 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (84)
📝 WalkthroughWalkthroughThe change centralizes SPA API configuration through application parameters and a dedicated DOM element. Controllers no longer pass API values as template props. PHPUnit uses dotenv-aware bootstrapping, and integration tests register requests with 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)
✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Integration/Controller/CampaignsControllerTest.php (1)
41-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the configured API URL in this test.
The assertion at Lines 58-60 still hard-codes
http://api.phplist.local/, unlikeAnalyticsControllerTest.phpandBouncesControllerTest.php. Readapp.api_base_urlfrom the container and interpolate it so the test remains valid when configuration differs in CI or another environment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Integration/Controller/CampaignsControllerTest.php` around lines 41 - 45, Update the test setup in CampaignsControllerTest to read app.api_base_url from the container and use that configured value in the assertion currently hard-coding http://api.phplist.local/. Match the configured-URL handling used by AnalyticsControllerTest and BouncesControllerTest while preserving the existing assertion behavior.
🤖 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.
Outside diff comments:
In `@tests/Integration/Controller/CampaignsControllerTest.php`:
- Around line 41-45: Update the test setup in CampaignsControllerTest to read
app.api_base_url from the container and use that configured value in the
assertion currently hard-coding http://api.phplist.local/. Match the
configured-URL handling used by AnalyticsControllerTest and
BouncesControllerTest while preserving the existing assertion behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cbb71002-bd7b-43f1-8ab4-16dc3b002fdc
📒 Files selected for processing (22)
assets/vue/api.jscomposer.jsonconfig/packages/twig.yamlconfig/services.ymlphpunit.xml.distsrc/Controller/AnalyticsController.phpsrc/Controller/BouncesController.phpsrc/Controller/CampaignsController.phpsrc/Controller/DashboardController.phpsrc/Controller/ListsController.phpsrc/Controller/PublicPagesController.phpsrc/Controller/SettingsController.phpsrc/Controller/SubscribersController.phpsrc/Controller/TemplatesController.phptemplates/base.html.twigtemplates/spa.html.twigtests/Integration/Controller/AnalyticsControllerTest.phptests/Integration/Controller/BouncesControllerTest.phptests/Integration/Controller/CampaignsControllerTest.phptests/Integration/Controller/DashboardControllerTest.phptests/Integration/Controller/ListsControllerTest.phptests/bootstrap.php
💤 Files with no reviewable changes (9)
- src/Controller/AnalyticsController.php
- src/Controller/DashboardController.php
- src/Controller/SettingsController.php
- src/Controller/BouncesController.php
- src/Controller/CampaignsController.php
- src/Controller/ListsController.php
- src/Controller/SubscribersController.php
- src/Controller/TemplatesController.php
- src/Controller/PublicPagesController.php
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openapi.json (1)
8221-8229: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep
requeuedin the status schema if requeuing is supported.
assets/vue/components/campaigns/CampaignDirectory.vuestill callscampaignClient.updateCampaignStatus(campaignId, 'requeued'), butopenapi.jsonno longer listsrequeuedinMessageMetadataRequest.status.enum. Add it back if the API accepts it, otherwise show another status or disable the Requeue action.🤖 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 `@openapi.json` around lines 8221 - 8229, Update MessageMetadataRequest.status and the Requeue action in CampaignDirectory.vue so they remain consistent: if updateCampaignStatus supports requeued, add "requeued" back to the schema enum; otherwise replace or disable the action and use an accepted status.
🤖 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.
Outside diff comments:
In `@openapi.json`:
- Around line 8221-8229: Update MessageMetadataRequest.status and the Requeue
action in CampaignDirectory.vue so they remain consistent: if
updateCampaignStatus supports requeued, add "requeued" back to the schema enum;
otherwise replace or disable the action and use an accepted status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 25b3d077-9f84-4192-a0f5-117fa6cbdf37
📒 Files selected for processing (4)
assets/vue/views/TemplateEditView.vuecomposer.jsonopenapi.jsontests/Unit/assets/vue/views/TemplateEditorView.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- composer.json
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/App.vue`:
- Around line 12-20: Update the App.vue startup flow around useDarkMode so
isDark is initialized before any child render, rather than waiting for
onMounted. Either have initDarkMode read
document.documentElement.classList.contains('dark') with a browser/SSR guard, or
call that initialization before mounting the AppSidebar/other children. Keep
AdminLayout.vue’s toggle state and aria-label in sync with the initial dark-mode
class on first render.
In `@assets/vue/components/base/BaseIcon.vue`:
- Around line 115-117: Update the SVG registry entries for warning, template,
public, list, and copy in BaseIcon so their child SVG color classes inherit the
wrapper’s selected and hover colors, including the matching dark variants;
remove or replace fixed text-slate color classes that override wrapper styling
while preserving each icon’s existing state behavior.
In `@assets/vue/components/settings/SettingsConfigs.vue`:
- Line 46: Update the non-editable branch of the :class binding in
SettingsConfigs.vue to replace dark:text-slate-500 with a lighter dark-mode text
token, while preserving the existing read-only background and other classes.
In `@assets/vue/components/sidebar/SidebarNavSection.vue`:
- Line 3: Replace the insufficient dark-mode slate-500 text token with slate-400
or another tested higher-contrast token at
assets/vue/components/sidebar/SidebarNavSection.vue lines 3-3,
assets/vue/components/bounces/BounceRules.vue lines 43-43,
assets/vue/views/CampaignEditView.vue lines 125-125, and
assets/vue/views/CampaignEditView.vue lines 260-262; update the corresponding
section label, list-order value, “No template” option, and test-recipient helper
text while preserving their existing light-mode styling and behavior.
In `@assets/vue/views/AnalyticsView.vue`:
- Around line 35-49: Update the secondary analytics count labels in
AnalyticsView, including the campaign count and corresponding labels near the
referenced sections, from dark:text-slate-500 to dark:text-slate-400 for
improved contrast. Do not change the chart x-axis and y-axis dark-mode color
handling.
🪄 Autofix
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 Plus
Run ID: 7e852151-d2d3-42e5-892b-0826cb3d105e
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (57)
assets/editor/EditorAssetPicker.vueassets/styles/app.cssassets/vue/App.vueassets/vue/components/base/BaseBadge.vueassets/vue/components/base/BaseButton.vueassets/vue/components/base/BaseCard.vueassets/vue/components/base/BaseIcon.vueassets/vue/components/base/BaseProgressBar.vueassets/vue/components/base/CkEditorField.vueassets/vue/components/bounces/BounceOverview.vueassets/vue/components/bounces/BouncePer.vueassets/vue/components/bounces/BounceRules.vueassets/vue/components/bounces/BouncesActionsPanel.vueassets/vue/components/campaigns/CampaignDirectory.vueassets/vue/components/campaigns/ViewCampaignModal.vueassets/vue/components/dashboard/CampaignsTable.vueassets/vue/components/dashboard/KpiCard.vueassets/vue/components/dashboard/PerformanceChartCard.vueassets/vue/components/dashboard/QuickActionsCard.vueassets/vue/components/dashboard/RecentCampaignsCard.vueassets/vue/components/lists/AddSubscribersModal.vueassets/vue/components/lists/CreateListModal.vueassets/vue/components/lists/EditListModal.vueassets/vue/components/lists/ListDirectory.vueassets/vue/components/lists/ListSubscribersExportPanel.vueassets/vue/components/public-pages/PublicPageEditor.vueassets/vue/components/public-pages/PublicPagesDirectory.vueassets/vue/components/settings/CreateAdminAttributeModal.vueassets/vue/components/settings/CreateAdminModal.vueassets/vue/components/settings/CreateSubscriberAttributeModal.vueassets/vue/components/settings/EditAdminAttributeModal.vueassets/vue/components/settings/EditAdminModal.vueassets/vue/components/settings/EditSubscriberAttributeModal.vueassets/vue/components/settings/SettingsActionsPanel.vueassets/vue/components/settings/SettingsAdminAttributes.vueassets/vue/components/settings/SettingsAdmins.vueassets/vue/components/settings/SettingsConfigs.vueassets/vue/components/settings/SettingsSubscriberAttributes.vueassets/vue/components/sidebar/AppSidebar.vueassets/vue/components/sidebar/SidebarNavItem.vueassets/vue/components/sidebar/SidebarNavSection.vueassets/vue/components/subscribers/ImportResult.vueassets/vue/components/subscribers/SubscriberDirectory.vueassets/vue/components/subscribers/SubscriberFilters.vueassets/vue/components/subscribers/SubscriberModal.vueassets/vue/components/subscribers/SubscriberTable.vueassets/vue/components/templates/TemplateLibrary.vueassets/vue/composables/useDarkMode.jsassets/vue/layouts/AdminLayout.vueassets/vue/views/AnalyticsView.vueassets/vue/views/CampaignEditView.vueassets/vue/views/DashboardView.vueassets/vue/views/ListSubscribersView.vueassets/vue/views/TemplateEditView.vuepackage.jsontemplates/base.html.twigtests/Unit/assets/vue/components/campaigns/CampaignDirectory.spec.js
Comment on lines
+115
to
+117
| classes.push("text-ext-wf3 dark:text-indigo-400"); | ||
| } else { | ||
| classes.push("text-slate-400 group-hover:text-slate-600"); | ||
| classes.push("text-slate-400 group-hover:text-slate-600 dark:group-hover:text-slate-300"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)BaseIcon\.vue$|base/SvgRegistry|SvgRegistry|IconRegistry|icon.*registry' || true
echo
echo "BaseIcon outline/size:"
if [ -f assets/vue/components/base/BaseIcon.vue ]; then
wc -l assets/vue/components/base/BaseIcon.vue
ast-grep outline assets/vue/components/base/BaseIcon.vue --view expanded || true
fi
echo
echo "Context around lines 100-130:"
sed -n '1,180p' assets/vue/components/base/BaseIcon.vue
echo
echo "Search registry entries and text classes:"
rg -n "warning|template|public|list|copy|text-slate-500|wrapperClass|innerClass|svgClass|color|dark:text-indigo|text-ext-wf3|group-hover:text-slate-300" assets/vue/components/base -S || trueRepository: phpList/web-frontend
Length of output: 22641
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("assets/vue/components/base/BaseIcon.vue")
if p.exists():
text = p.read_text()
state_classes = [
"text-ext-wf3",
"dark:text-indigo-400",
"group-hover:text-slate-600",
"dark:group-hover:text-slate-300",
"dark:text-slate-400"
]
entries = ["warning", "template", "public", "list", "copy"]
print("Has relevant state class in BaseIcon.vue:", any(c in text for c in state_classes))
for entry in entries:
idx = text.find(entry)
print(f"\nINDEX {entry}: {idx}")
if idx != -1:
lo=max(0, idx-500); hi=min(len(text), idx+500)
print(text[lo:hi])
else:
print("file missing")
PYRepository: phpList/web-frontend
Length of output: 4808
Make inner SVG icons inherit or inherit wrapper colors.
BaseIcon applies dark selected/hover colors on the wrapper <span>, but registries like warning, template, public, list, and copy set a child SVG’s own text-slate-500 and bypass the wrapper state colors. Add the matching dark state variants to those SVG entries, or use wrapper-only color variants in the registry.
🤖 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/base/BaseIcon.vue` around lines 115 - 117, Update the
SVG registry entries for warning, template, public, list, and copy in BaseIcon
so their child SVG color classes inherit the wrapper’s selected and hover
colors, including the matching dark variants; remove or replace fixed text-slate
color classes that override wrapper styling while preserving each icon’s
existing state behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Thanks for contributing to phpList!