Move vue-resource to axios and consolidate #1157
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughCentralized HTTP client by adding a shared Axios instance with CSRF handling, removed vue-resource usage and scattered CSRF initializers, updated API calls to use the new instance and switched response access from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
💤 Files with no reviewable changes (6)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
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 |
lodewiges
added
refactor
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## staging #1157 +/- ##
========================================
Coverage 77.62% 77.62%
========================================
Files 54 54
Lines 1341 1341
========================================
Hits 1041 1041
Misses 300 300 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR migrates the project from vue-resource to axios as a step toward Vue 3 compatibility. The migration consolidates HTTP client configuration into a centralized axios instance with consistent CSRF token handling and JSON response headers.
- Removed
vue-resourcedependency and all its transitive dependencies - Created a centralized axios instance in
app/javascript/api/axiosInstance.jswith CSRF token configuration - Updated all Vue components and JavaScript files to use the new axios instance instead of
this.$httpor direct axios imports - Updated API response access from
response.bodytoresponse.data(axios convention) - Improved placeholder text in user input fields from "Begin met typen..." to "Zoek op gebruiker" and "Zoek op prijslijst"
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Removed vue-resource dependency |
| yarn.lock | Cleaned up vue-resource and its transitive dependencies (got, cacheable-request, etc.) |
| app/javascript/api/axiosInstance.js | Created centralized axios instance with CSRF token and Accept header configuration |
| app/javascript/users.js | Removed manual CSRF token setup, imported centralized api instance |
| app/javascript/user.js | Removed manual CSRF token setup, imported centralized api instance |
| app/javascript/price_lists.js | Migrated from VueResource to axios, updated all HTTP calls and response handling |
| app/javascript/payment_add.js | Removed manual CSRF token setup, imported centralized api instance |
| app/javascript/order_screen.js | Migrated from VueResource to axios, updated all HTTP calls and response handling |
| app/javascript/invoices.js | Removed VueResource usage, imported centralized api instance |
| app/javascript/credit_mutations.js | Removed VueResource usage |
| app/javascript/activity.js | Removed VueResource usage, imported centralized api instance |
| app/javascript/activities.js | Removed VueResource usage, imported centralized api instance |
| app/javascript/components/user/OrderHistory.vue | Updated axios import to use centralized api instance |
| app/javascript/components/user/ActivityOrderHistory.vue | Updated axios import to use centralized api instance |
| app/javascript/components/orderscreen/ActivityOrders.vue | Updated axios import to use centralized api instance |
| app/javascript/components/activity/ProductTotals.vue | Migrated from this.$http to axios, updated response handling |
| app/javascript/components/UserInput.vue | Migrated from this.$http to axios, updated response handling, improved placeholder text |
| app/javascript/components/ProductTable.vue | Updated axios import to use centralized api instance |
| app/views/activities/_modal.html.erb | Updated placeholder text from "Begin met typen..." to "Zoek op prijslijst" |
Comments suppressed due to low confidence (6)
app/javascript/activity.js:3
- Unused import api.
import api from './api/axiosInstance';
app/javascript/activities.js:3
- Unused import api.
import api from './api/axiosInstance';
app/javascript/invoices.js:3
- Unused import api.
import api from './api/axiosInstance';
app/javascript/payment_add.js:3
- Unused import api.
import api from './api/axiosInstance';
app/javascript/user.js:3
- Unused import api.
import api from './api/axiosInstance';
app/javascript/users.js:3
- Unused import api.
import api from './api/axiosInstance';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/javascript/invoices.js
Outdated
| // eslint-disable-next-line no-unused-vars | ||
| import api from './api/axiosInstance'; |
There was a problem hiding this comment.
The import of 'api' appears to be unused in this file. If the api instance is only used by child components (like UserInput), those components should import it directly. This import can be removed from this file to avoid confusion and the need for the eslint-disable comment.
| // eslint-disable-next-line no-unused-vars | |
| import api from './api/axiosInstance'; |
app/javascript/payment_add.js
Outdated
| // eslint-disable-next-line no-unused-vars | ||
| import api from './api/axiosInstance'; |
There was a problem hiding this comment.
The import of 'api' appears to be unused in this file. If the api instance is only used by child components, those components should import it directly. This import can be removed from this file to avoid confusion and the need for the eslint-disable comment.
| // eslint-disable-next-line no-unused-vars | |
| import api from './api/axiosInstance'; |
app/javascript/activities.js
Outdated
| // eslint-disable-next-line no-unused-vars | ||
| import api from './api/axiosInstance'; |
There was a problem hiding this comment.
The import of 'api' appears to be unused in this file. If the api instance is only used by child components, those components should import it directly. This import can be removed from this file to avoid confusion and the need for the eslint-disable comment.
| // eslint-disable-next-line no-unused-vars | |
| import api from './api/axiosInstance'; |
app/javascript/api/axiosInstance.js
Outdated
| return instance; | ||
| } | ||
|
|
||
| export default createAxiosInstance(); |
There was a problem hiding this comment.
The axios instance is created at module load time, which means the CSRF token is read immediately when this module is first imported. In a Turbo application, the CSRF token meta tag might not be available in the DOM at module load time, especially during the initial page load or when JavaScript modules are loaded before the DOM is ready. This could result in requests being made without a CSRF token, causing authentication failures. Consider lazily initializing the instance on first use, or re-reading the CSRF token before each request using an axios interceptor.
app/javascript/users.js
Outdated
| // eslint-disable-next-line no-unused-vars | ||
| import api from './api/axiosInstance'; |
There was a problem hiding this comment.
The import of 'api' appears to be unused in this file. If the api instance is only used by child components (like UsersTable), those components should import it directly. This import can be removed from this file to avoid confusion and the need for the eslint-disable comment.
| // eslint-disable-next-line no-unused-vars | |
| import api from './api/axiosInstance'; |
app/javascript/user.js
Outdated
| // eslint-disable-next-line no-unused-vars | ||
| import api from './api/axiosInstance'; |
There was a problem hiding this comment.
The import of 'api' appears to be unused in this file. If the api instance is only used by child components (like UserInput), those components should import it directly. This import can be removed from this file to avoid confusion and the need for the eslint-disable comment.
| // eslint-disable-next-line no-unused-vars | |
| import api from './api/axiosInstance'; |
app/javascript/activity.js
Outdated
| // eslint-disable-next-line no-unused-vars | ||
| import api from './api/axiosInstance'; |
There was a problem hiding this comment.
The import of 'api' appears to be unused in this file. If the api instance is only used by child components (like ProductTotals), those components should import it directly. This import can be removed from this file to avoid confusion and the need for the eslint-disable comment.
| // eslint-disable-next-line no-unused-vars | |
| import api from './api/axiosInstance'; |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/javascript/components/UserInput.vue (1)
81-93: Missing error handling for the search request.The
api.postcall lacks a.catch()handler. If the search request fails (network error, server error), the promise rejection will be unhandled and the user receives no feedback. The suggestions list may remain stale or empty without indication of the failure.Suggested fix
api.post('/users/search.json', { query: this.query }).then( (response) => { let results = response.data || []; if (this.includePin && 'gepind'.indexOf(this.query.toLowerCase()) >= 0) { results.push({ name: 'Gepind', paid_with_pin: true }); } if (this.includeCash && 'contant betaald'.indexOf(this.query.toLowerCase()) >= 0) { results.push({ name: 'Contant betaald', paid_with_cash: true }); } this.suggestions = results; this.open = true; + }).catch(() => { + this.suggestions = []; });app/javascript/order_screen.js (1)
202-218: Fix error body reference for axios.Line 207 references
error.body, but axios structures errors differently. With axios, the response body is aterror.response.data, noterror.body. This will cause undefined values to be sent to Sentry.🔎 Proposed fix
try { - throw new Error(JSON.stringify(error.body)); + throw new Error(JSON.stringify(error.response?.data || error.message)); } catch(e) {
🧹 Nitpick comments (4)
app/javascript/components/user/OrderHistory.vue (1)
79-79: Consider using template literals for URL construction.The migration from
axios.gettoapi.getis correct. However, the URL string concatenation could be more readable with template literals.🔎 Optional refactor using template literals
- let promise = api.get('/users/'+this.user.id+'/activities'); + let promise = api.get(`/users/${this.user.id}/activities`);app/javascript/payment_add.js (1)
2-3: Unused import with eslint suppression is a code smell.The
apiimport is marked as unused, and indeed this file makes no HTTP requests. If the import is needed for CSRF token setup side effects, consider either:
- Removing this import if CSRF setup happens elsewhere in the application bootstrap
- Making the side-effect explicit with a comment explaining why it's needed
If no HTTP calls are made from this module, the import can likely be removed entirely.
Suggested change if import is not needed
-// eslint-disable-next-line no-unused-vars -import api from './api/axiosInstance';app/javascript/invoices.js (1)
2-3: Same unused import pattern - consider removing.Similar to
payment_add.js, this file importsapibut never uses it. The Vue instance handles only local state (activity selection, row manipulation) without any HTTP calls.Suggested change
-// eslint-disable-next-line no-unused-vars -import api from './api/axiosInstance';app/javascript/activities.js (1)
2-3: Remove the unused import.The
apiimport is not used anywhere in this file. Remove both the import statement and the eslint-disable comment to keep the code clean and reduce bundle size.🔎 Proposed fix
-// eslint-disable-next-line no-unused-vars -import api from './api/axiosInstance';
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
app/javascript/activities.jsapp/javascript/activity.jsapp/javascript/api/axiosInstance.jsapp/javascript/components/ProductTable.vueapp/javascript/components/UserInput.vueapp/javascript/components/activity/ProductTotals.vueapp/javascript/components/orderscreen/ActivityOrders.vueapp/javascript/components/user/ActivityOrderHistory.vueapp/javascript/components/user/OrderHistory.vueapp/javascript/credit_mutations.jsapp/javascript/invoices.jsapp/javascript/order_screen.jsapp/javascript/payment_add.jsapp/javascript/price_lists.jsapp/javascript/user.jsapp/javascript/users.jsapp/views/activities/_modal.html.erbpackage.json
💤 Files with no reviewable changes (2)
- package.json
- app/javascript/credit_mutations.js
🧰 Additional context used
🧬 Code graph analysis (1)
app/javascript/price_lists.js (2)
app/controllers/orders_controller.rb (1)
index(6-16)app/controllers/activities_controller.rb (1)
index(11-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (18)
app/views/activities/_modal.html.erb (1)
25-25: LGTM! Placeholder text improvement aligns with UX goal.The updated placeholder text "Zoek op prijslijst" is clearer and more action-oriented than "Begin met typen...", making the search functionality more intuitive for users.
app/javascript/components/activity/ProductTotals.vue (2)
53-53: LGTM! Centralized API import added correctly.
81-82: LGTM! HTTP client migration is correct.The migration from
this.$http.get(vue-resource) toapi.get(axios) and updating response handling fromresponse.bodytoresponse.datafollows the correct pattern for this refactor.app/javascript/components/ProductTable.vue (2)
45-45: LGTM! Centralized API import added correctly.
88-88: LGTM! Migration to centralized API instance is correct.The change from direct
axios.patchtoapi.patchproperly adopts the centralized axios instance with unified CSRF token handling.app/javascript/components/user/OrderHistory.vue (1)
58-58: LGTM! Centralized API import added correctly.app/javascript/components/orderscreen/ActivityOrders.vue (2)
64-64: LGTM! Centralized API import added correctly.
100-100: LGTM! Migration to centralized API instance is correct.The change from direct
axios.gettoapi.getproperly adopts the centralized axios instance with unified CSRF token handling.app/javascript/activity.js (1)
2-3: Theapiimport is necessary and should not be removed.The axiosInstance module has initialization side effects that are required for the application to function. When imported, it retrieves the CSRF token from the DOM and configures the axios instance with security headers. The eslint-disable comment is appropriate as-is since the import triggers these side effects even though the
apivariable is not directly referenced.Likely an incorrect or invalid review comment.
app/javascript/components/user/ActivityOrderHistory.vue (1)
20-20: Migration to centralized API instance looks correct.The switch from direct axios to the centralized
apiinstance is properly implemented. The response handling correctly usesresponse.data, and the error callback handles loading state.Consider adding user feedback on error (e.g., displaying an error message) rather than silently hiding the loading state, though this may be intentional for this use case.
Also applies to: 45-52
app/javascript/components/UserInput.vue (2)
4-8: Placeholder text update addresses issue #1185.The change from "Begin met typen..." to "Zoek op gebruiker" makes the search behavior more intuitive as requested.
Minor nit:
required="true"works but the idiomatic HTML boolean attribute is justrequired.
22-23: API import migration looks correct.The switch to the centralized API instance is properly implemented.
app/javascript/price_lists.js (3)
2-2: Clean import of centralized API instance.Unlike other files in this PR, this import is actually used throughout the file.
71-91: Product save operations properly migrated with error handling.The migration to
api.putandapi.postis correctly implemented. Error handling uses optional chaining (error.response?.data?.errors) which safely handles cases where the response structure might differ.
144-158: Archive/unarchive operations properly migrated.Both operations use
api.postwith appropriate error handling. The backend correctly returns the timestamp directly viarender json: @price_list.archived_at, so the assignmentpriceList.archived_at = response.dataworks as expected.app/javascript/order_screen.js (2)
2-2: LGTM!The centralized API import correctly replaces the previous vue-resource setup.
61-76: HTTP migration looks good overall.The migration from vue-resource to the centralized axios instance is consistent throughout, and all response handling correctly uses
response.datainstead ofresponse.body.Also applies to: 158-199, 243-248, 340-374
app/javascript/api/axiosInstance.js (1)
1-1: axios 1.13.2 is valid and secure.Version 1.13.2 is the latest version, and no direct vulnerabilities have been found for this package in Snyk's vulnerability database.
app/javascript/user.js
Outdated
| // eslint-disable-next-line no-unused-vars | ||
| import api from './api/axiosInstance'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if 'api' is used anywhere in user.js beyond the import
echo "=== Checking 'api' usage in user.js ==="
rg -n '\bapi\b' app/javascript/user.js
# Verify axiosInstance module
echo -e "\n=== Checking axiosInstance module ==="
if [ -f "app/javascript/api/axiosInstance.js" ]; then
cat -n app/javascript/api/axiosInstance.js
else
find . -name "axiosInstance*" -type f 2>/dev/null | head -5
fi
# Check OrderHistory component imports
echo -e "\n=== Checking OrderHistory component imports ==="
rg -n 'import.*axiosInstance|import.*api' app/javascript/components/user/OrderHistory.vue 2>/dev/null || \
rg -n 'import.*axiosInstance|import.*api' app/javascript/components/ -A 2 | head -20Repository: csvalpha/sofia
Length of output: 1304
Add a comment explaining why the axiosInstance import is necessary despite being unused in this file.
The api import is not used in user.js, but the axiosInstance module has necessary initialization side effects—it retrieves the CSRF token from the meta tag and configures the axios instance with security headers. Since this initialization is required for the app's CSRF protection, replace the generic eslint-disable comment with one that clarifies this:
// Import axiosInstance to initialize CSRF token configuration
import api from './api/axiosInstance';Alternatively, keep the eslint-disable but add context:
// eslint-disable-next-line no-unused-vars
// Import required for axiosInstance initialization side effects (CSRF token setup)
import api from './api/axiosInstance';🤖 Prompt for AI Agents
In app/javascript/user.js around lines 2-3, the import "api from
'./api/axiosInstance'" is unused but kept for its initialization side-effects
(CSRF token retrieval and axios configuration); replace the vague eslint-disable
comment with a clear explanatory comment stating that the import is required to
initialize CSRF/security headers (or keep the eslint-disable and add that
explanation) so future readers understand why the unused import is intentional.
This PR is the first stap to moving to Vue 3
fixes #1107
also fixes #1185 because it was a small change
Summary by CodeRabbit
Style
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.