Update/migrate webpacker to js bundeling - #1108
Conversation
|
Warning Rate limit exceeded@lodewiges has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis PR comprehensively migrates the application from Turbolinks to Turbo, replaces Webpacker with native Rails 7+ asset bundling (jsbundling, cssbundling), updates the JavaScript build chain (webpack, Babel), modernizes linting tools (ESLint flat config, Stylelint), and updates FontAwesome icon usage across views and components. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Browser
participant OldApp as App (Turbolinks)
participant NewApp as App (Turbo)
rect rgb(200, 220, 255)
note over OldApp: Old Flow
User->>Browser: Click link
Browser->>OldApp: turbolinks:load
OldApp->>OldApp: Initialize Vue + TurbolinksAdapter
OldApp-->>Browser: Render page
User->>Browser: Navigate away
Browser->>OldApp: turbolinks:before-cache
OldApp->>OldApp: Cleanup (if implemented)
end
rect rgb(200, 255, 220)
note over NewApp: New Flow
User->>Browser: Click link
Browser->>NewApp: turbo:load
NewApp->>NewApp: Initialize Vue (no adapter)
NewApp-->>Browser: Render page
User->>Browser: Navigate away
Browser->>NewApp: turbo:before-cache
NewApp->>NewApp: Destroy Vue instance
end
sequenceDiagram
autonumber
participant Build as Build System
participant Webpack as Webpack
participant Assets as app/assets/builds
rect rgb(220, 240, 255)
note over Build: Old Webpacker Pipeline
Build->>Webpack: webpacker gem
Webpack->>Webpack: Process packs/<entry>.js
Webpack-->>Assets: public/packs/<entry>.js
end
rect rgb(255, 240, 220)
note over Build: New Rails 7+ Pipeline
Build->>Webpack: native webpack.config.js
Webpack->>Webpack: Process app/javascript/*.js entries
Webpack-->>Assets: app/assets/builds/<entry>.js
Assets->>Assets: Sprockets compiles & serves
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## staging #1108 +/- ##
===========================================
+ Coverage 74.10% 75.27% +1.17%
===========================================
Files 51 50 -1
Lines 1093 1076 -17
===========================================
Hits 810 810
+ Misses 283 266 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/javascript/price_lists.js (1)
116-125: ReplaceVue.util.extendwithObject.assignor spread operator.
Vue.utilis an internal API not intended for public use and may change in future Vue versions. UseObject.assignor the spread operator for shallow cloning instead.Apply this diff:
editProduct: function(product) { // Save original state - product._beforeEditingCache = Vue.util.extend({}, product); + product._beforeEditingCache = Object.assign({}, product); product.product_prices.forEach((pp, i) => { - product._beforeEditingCache.product_prices[i] = Vue.util.extend({}, pp); + product._beforeEditingCache.product_prices[i] = Object.assign({}, pp); }); product.editing = true; return product; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/javascript/price_lists.js(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/javascript/price_lists.js (8)
app/javascript/order_screen.js (1)
element(9-9)app/javascript/invoices.js (1)
element(16-16)app/javascript/credit_mutations.js (1)
element(16-16)app/javascript/payment_add.js (1)
element(6-6)app/javascript/users.js (1)
element(14-14)app/javascript/user.js (1)
element(7-7)app/javascript/activity.js (1)
element(15-15)app/javascript/activities.js (2)
element(4-4)priceLists(6-6)
⏰ 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
| if (sanitizedProduct.id) { // Existing product | ||
| this.$http.put(`/products/${sanitizedProduct.id}.json`, { product: sanitizedProduct }).then( (response) => { | ||
| var newProduct = response.data; | ||
| this.$http.put(`/products/${sanitizedProduct.id}.json`, { product: sanitizedProduct }).then((response) => { |
There was a problem hiding this comment.
this.$http is undefined — vue-resource is not imported or configured.
The code uses this.$http.put (line 64), this.$http.post (lines 73, 139, 147), but vue-resource is neither imported nor configured in this file. This will cause runtime errors when these methods are called. Past review comments indicate axios was intended (commit 548e00f), but the HTTP calls were never migrated from this.$http to axios.
Install vue-resource and configure it globally, or migrate to axios. Given the modernization goals of this PR, migrating to axios is recommended:
Step 1: Import axios at the top of the file:
import Vue from 'vue/dist/vue.esm';
+import axios from 'axios';Step 2: Replace this.$http calls with axios. For example, line 64:
-this.$http.put(`/products/${sanitizedProduct.id}.json`, { product: sanitizedProduct }).then((response) => {
- const newProduct = response.body;
+axios.put(`/products/${sanitizedProduct.id}.json`, { product: sanitizedProduct }).then((response) => {
+ const newProduct = response.data;
newProduct.editing = false;
this.$set(this.products, this.products.indexOf(product), newProduct);
}).catch((error) => {
- this.errors = error.response?.body?.errors || ['An error occurred'];
+ this.errors = error.response?.data?.errors || ['An error occurred'];
});Apply the same pattern to lines 73, 139, and 147 (replace response.body with response.data and error.response?.body with error.response?.data).
Also applies to: 73-73, 139-139, 147-147
🤖 Prompt for AI Agents
In app/javascript/price_lists.js around lines 64, 73, 139 and 147, the code uses
this.$http which is undefined because vue-resource isn't imported/configured;
migrate these calls to axios by importing axios at the top of the file and
replacing this.$http.put/post(...) with axios.put/post(...), update success
handling to use response.data instead of response.body, and update error
handling to use error.response?.data instead of error.response?.body; ensure the
promise chaining/async handling remains the same and remove any dependency on
this.$http throughout the file.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
06cfb6a to
f276172
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
app/javascript/price_lists.js (2)
143-148: Fix inconsistent response property access in archivePriceList.The method uses VueResource's
this.$http(line 143) but accessesresponse.data(line 144) anderror.response?.data?.errors(line 146), which are axios-style properties. VueResource usesresponse.body.Apply this diff:
this.$http.post(`/price_lists/${priceList.id}/archive`, {}).then((response) => { - priceList.archived_at = response.data; + priceList.archived_at = response.body; }).catch((error) => { - this.errors = error.response?.data?.errors || ['An error occurred']; + this.errors = error.body?.errors || ['An error occurred']; });
150-156: Fix inconsistent response property access in unarchivePriceList.Same issue as
archivePriceList: the method uses VueResource but accesses axios-style response properties.Apply this diff:
this.$http.post(`/price_lists/${priceList.id}/unarchive`, {}).then((response) => { - priceList.archived_at = response.data; + priceList.archived_at = response.body; }).catch((error) => { - this.errors = error.response?.data?.errors || ['An error occurred']; + this.errors = error.body?.errors || ['An error occurred']; });
🧹 Nitpick comments (3)
app/javascript/components/activity/ProductTotals.vue (1)
84-87: Consider adding user-facing error feedback.The error handler correctly stops the spinner and logs the error, but users receive no indication that the request failed. When an error occurs, the component displays the empty-state message "Er zijn nog geen producten verkocht," which misleadingly suggests there's no data rather than a failure to load. Consider adding an error state to inform users of the issue.
For example, add an error data property and display it in the template:
data() { return { user: {}, orderTotals: [], totalAmount: 0.0, - isLoading: true + isLoading: true, + error: null }; },loadProductTotals() { this.isLoading = true; + this.error = null; let params = {user: this.user.id, paid_with_cash: this.user.paid_with_cash, paid_with_pin: this.user.paid_with_pin}; this.$http.get(`/activities/${this.activity}/product_totals`, { params }).then((response) => { this.orderTotals = response.body; this.totalAmount = this.orderTotals.reduce((a, b) => a + parseFloat(b.price), 0.0); this.isLoading = false; }).catch((error) => { this.isLoading = false; + this.error = 'Er is een fout opgetreden bij het laden van de producttotalen.'; console.error(error); }); },And update the template to show the error when present:
<div class="alert alert-danger" v-if="error"> {{ error }} </div>app/javascript/application.js (1)
22-22: Remove empty event handler or document its purpose.The empty
turbo:loadhandler serves no function. If this is a placeholder for future code, add a comment explaining that; otherwise, remove it.Apply this diff if no future use is planned:
-document.addEventListener('turbo:load', () => {});app/javascript/price_lists.js (1)
1-164: Consider completing the migration from VueResource to axios.Past review comments suggested migrating from VueResource (
this.$http) to axios for modernization. The current code still uses VueResource throughout. While the immediate fixes above will resolve the response property inconsistencies, completing the axios migration would:
- Align with modern JavaScript ecosystem standards (VueResource is no longer maintained for Vue 2)
- Be consistent with other modules in the codebase that use axios (e.g.,
user.js)- Reduce the number of HTTP client libraries in the bundle
This can be deferred if there are time constraints, but should be tracked for a follow-up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/javascript/activities.js(3 hunks)app/javascript/activity.js(1 hunks)app/javascript/application.js(1 hunks)app/javascript/components/activity/ProductTotals.vue(1 hunks)app/javascript/credit_mutations.js(1 hunks)app/javascript/invoices.js(1 hunks)app/javascript/payment_add.js(1 hunks)app/javascript/price_lists.js(5 hunks)app/javascript/user.js(1 hunks)app/javascript/users.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/javascript/credit_mutations.js
- app/javascript/invoices.js
- app/javascript/payment_add.js
- app/javascript/activity.js
🧰 Additional context used
🧬 Code graph analysis (3)
app/javascript/activities.js (1)
app/javascript/price_lists.js (2)
element(8-8)priceLists(10-10)
app/javascript/price_lists.js (1)
app/javascript/activities.js (2)
element(7-7)priceLists(9-9)
app/javascript/users.js (3)
app/javascript/activity.js (2)
vueInstance(7-7)element(18-18)app/javascript/credit_mutations.js (2)
vueInstance(8-8)element(20-20)app/javascript/invoices.js (2)
vueInstance(8-8)element(19-19)
🪛 ESLint
app/javascript/application.js
[error] 6-6: 'axios' is defined but never used.
(no-unused-vars)
⏰ 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 (5)
app/javascript/users.js (2)
5-12: LGTM!The Vue instance tracking and cleanup logic properly handles Turbo's caching mechanism, preventing memory leaks and stale instances. The pattern is consistent with other entrypoints in the codebase.
16-17: LGTM!The element selection properly checks for
nullbefore proceeding, ensuring the Vue initialization only runs on the appropriate page.app/javascript/user.js (1)
4-12: LGTM! Clean migration to Turbo.The changes correctly update the import path, migrate from
turbolinks:loadtoturbo:load, and modernize variable declarations fromvartoconst. All changes align with the PR's migration objectives.app/javascript/activities.js (1)
5-9: Good defensive programming with the element guard.The migration to
turbo:loadis correct, and the added null check on line 8 prevents errors when the target element is not present on the page.app/javascript/price_lists.js (1)
68-88: Fix inconsistent response property access in saveProduct.The code uses VueResource's
this.$httpAPI (lines 68, 77) but mixes response property styles:
- Lines 69, 81:
response.data(axios style)- Line 74:
error.response?.data?.errors(axios style)- Line 86:
error.response?.body?.errors(VueResource style)VueResource uses
response.bodyfor success andresponse.bodyfor errors, notresponse.data. This inconsistency will cause silent failures where response data is undefined or errors aren't captured correctly.Apply this diff to use VueResource properties consistently:
this.$http.put(`/products/${sanitizedProduct.id}.json`, { product: sanitizedProduct }).then((response) => { - const newProduct = response.data; + const newProduct = response.body; newProduct.editing = false; this.$set(this.products, this.products.indexOf(product), newProduct); }).catch((error) => { - this.errors = error.response?.data?.errors || ['An error occurred']; + this.errors = error.body?.errors || ['An error occurred']; });Apply the same fix to the
postcall:this.$http.post('/products.json', { product: sanitizedProduct }).then( (response) => { const index = this.products.indexOf(product); this.products.splice(index, 1); - const newProduct = response.data; + const newProduct = response.body; newProduct.editing = false; this.products.push(newProduct); }).catch((error) => { - this.errors = error.response?.body?.errors || ['An error occurred']; + this.errors = error.body?.errors || ['An error occurred']; });Likely an incorrect or invalid review comment.
| this.$http.get(`/activities/${this.activity}/product_totals`, { params }).then((response) => { | ||
| this.orderTotals = response.body; | ||
| this.totalAmount = this.orderTotals.reduce((a, b) => a + parseFloat(b.price), 0.0); | ||
| this.isLoading = false; | ||
| }).catch((error) => { | ||
| this.isLoading = false; | ||
| console.error(error); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify VueResource vs axios migration status.
The past review comment mentions axios and shows axios code (marked as addressed in commit 7a5b9c9), and the PR objectives indicate migration away from VueResource. However, this code still uses VueResource's this.$http API and accesses response.body (line 81). Please confirm whether:
- The axios migration was intentionally reverted or is incomplete for this component
- VueResource remains a dependency in the project
- The error handling pattern here aligns with the intended HTTP client library
Run the following script to check the HTTP client library usage across Vue components:
🏁 Script executed:
#!/bin/bash
# Check which HTTP client library is used in Vue components
echo "=== Checking for axios imports/usage in Vue components ==="
rg -n --type=vue -C2 "import.*axios|from.*axios|\baxios\."
echo -e "\n=== Checking for VueResource usage in Vue components ==="
rg -n --type=vue -C2 '\$http\.'
echo -e "\n=== Checking package.json for axios and vue-resource ==="
jq '.dependencies, .devDependencies' package.json | rg -i "axios|vue-resource"Length of output: 279
Let me run a corrected script to search the Vue files properly:
Wait, let me fix that - I need cloning for this:
🏁 Script executed:
#!/bin/bash
# Search Vue files using proper file path patterns
echo "=== Checking for axios usage in .vue files ==="
rg -n "import.*axios|from.*axios|axios\." app/javascript/components --type=js -g "*.vue"
echo -e "\n=== Checking for VueResource usage in .vue files ==="
rg -n '\$http\.' app/javascript/components -g "*.vue"
echo -e "\n=== List of .vue files in activity directory ==="
fd "\.vue$" app/javascript/components/activity
echo -e "\n=== Checking ProductTotals.vue for both libraries ==="
cat app/javascript/components/activity/ProductTotals.vue | head -100Length of output: 4015
🏁 Script executed:
#!/bin/bash
# Search Vue files using proper file path patterns
echo "=== Checking for axios usage in .vue files ==="
find app/javascript/components -name "*.vue" -exec grep -l "axios" {} \;
echo -e "\n=== Checking for VueResource usage in .vue files ==="
find app/javascript/components -name "*.vue" -exec grep -l '\$http' {} \;
echo -e "\n=== Count of .vue files using each library ==="
echo "Files using axios:"
find app/javascript/components -name "*.vue" -exec grep -l "axios" {} \; | wc -l
echo "Files using \$http:"
find app/javascript/components -name "*.vue" -exec grep -l '\$http' {} \; | wc -l
echo -e "\n=== Show axios vs VueResource usage pattern ==="
echo "Axios usage:"
find app/javascript/components -name "*.vue" -exec grep -l "axios" {} \; | head -5
echo -e "\nVueResource usage:"
find app/javascript/components -name "*.vue" -exec grep -l '\$http' {} \; | head -5Length of output: 866
Let me check how error handling is implemented in other components to determine if user-facing feedback is expected:
---
🏁 Script executed:
```shell
#!/bin/bash
# Compare error handling patterns in axios vs VueResource components
echo "=== Error handling in ProductTable.vue (axios) ==="
grep -A5 "\.catch" app/javascript/components/ProductTable.vue
echo -e "\n=== Error handling in UserInput.vue (VueResource) ==="
grep -A5 "\.catch" app/javascript/components/UserInput.vue
echo -e "\n=== Error handling in OrderHistory.vue (axios) ==="
grep -A5 "\.catch" app/javascript/components/user/OrderHistory.vue
echo -e "\n=== Error handling in ActivityOrders.vue (axios) ==="
grep -A5 "\.catch" app/javascript/components/orderscreen/ActivityOrders.vue
Length of output: 211
Let me check the actual HTTP request implementations more broadly:
---
🏁 Script executed:
```shell
#!/bin/bash
# Show HTTP request patterns in different components
echo "=== ProductTotals.vue HTTP request (VueResource, lines 80-87) ==="
sed -n '80,87p' app/javascript/components/activity/ProductTotals.vue
echo -e "\n=== ProductTable.vue HTTP request (axios) ==="
grep -B2 -A8 "axios.patch" app/javascript/components/ProductTable.vue | head -20
echo -e "\n=== UserInput.vue HTTP request (VueResource) ==="
grep -B2 -A8 "this.\$http.post" app/javascript/components/UserInput.vue | head -20
echo -e "\n=== ActivityOrders.vue HTTP request (axios) ==="
grep -B2 -A8 "axios.get" app/javascript/components/orderscreen/ActivityOrders.vue | head -20
echo -e "\n=== Checking if ProductTotals.vue has error handling in original file ==="
grep -n "catch\|error" app/javascript/components/activity/ProductTotals.vue
Length of output: 1981
Add user-facing error feedback to match component patterns in codebase.
The code correctly uses VueResource and implements error handling with console.error() (lines 84-87), but lacks user-facing feedback. Other components in the codebase—such as ProductTable.vue—display user-visible error messages (e.g., "Er is iets misgegaan bij het opslaan van deze rij"). Consider showing a similar message to the user when the product totals request fails, rather than silently logging to console.
The VueResource API usage is intentional as part of the ongoing gradual migration (4 components now use axios, 2 use VueResource; both libraries remain in dependencies).
🤖 Prompt for AI Agents
In app/javascript/components/activity/ProductTotals.vue around lines 80-87, the
catch block only logs the error to console and should show a user-facing error
like other components; add a data property (e.g. errorMessage or errorAlert) and
in the catch set this.errorMessage = "Er is iets misgegaan bij het ophalen van
producttotalen" (or use the same message used in ProductTable.vue), keep
this.isLoading = false and still console.error(error) for debugging, and update
the template to render the error message/alert in the same style/pattern as
ProductTable.vue so users see the failure.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
app/javascript/users.js (1)
1-3: Axios security vulnerabilities still need to be addressed.The previous review identified HIGH-severity vulnerabilities in axios 1.4.0 (DoS and SSRF/credential leakage). Please upgrade to axios >= 1.13.2 in
package.json.app/javascript/price_lists.js (1)
68-76: Incorrect response property access for VueResource.Line 69 accesses
response.dataafter athis.$http.putcall, but VueResource usesresponse.bodyfor the response payload, notresponse.data. Similarly, line 74's error handler useserror.response?.data?.errors, but VueResource errors useerror.response.body.If keeping VueResource, apply this diff:
this.$http.put(`/products/${sanitizedProduct.id}.json`, { product: sanitizedProduct }).then((response) => { - const newProduct = response.data; + const newProduct = response.body; newProduct.editing = false; this.$set(this.products, this.products.indexOf(product), newProduct); }).catch((error) => { - this.errors = error.response?.data?.errors || ['An error occurred']; + this.errors = error.response?.body?.errors || ['An error occurred']; });Alternatively, if migrating to axios, replace
this.$httpwithaxiosand keepresponse.data.
🧹 Nitpick comments (1)
app/javascript/users.js (1)
19-41: LGTM! Previous critical issue resolved.The JSON parsing is now wrapped in a try-catch block, addressing the previous review's concern about potential
SyntaxErrorcrashes. The error handling prevents page initialization failure when data attributes contain malformed JSON.Optional enhancement: Consider displaying a user-facing error message (e.g., setting a data property that renders an alert in the UI) in addition to the console logging, so users understand why the table didn't load.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/javascript/activities.js(3 hunks)app/javascript/application.js(1 hunks)app/javascript/components/activity/ProductTotals.vue(1 hunks)app/javascript/price_lists.js(5 hunks)app/javascript/users.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/javascript/application.js
🧰 Additional context used
🧬 Code graph analysis (3)
app/javascript/activities.js (1)
app/javascript/price_lists.js (2)
element(8-8)priceLists(10-10)
app/javascript/price_lists.js (1)
app/javascript/activities.js (2)
element(8-8)priceLists(10-10)
app/javascript/users.js (3)
app/javascript/credit_mutations.js (1)
vueInstance(8-8)app/javascript/invoices.js (1)
vueInstance(8-8)app/javascript/activity.js (1)
vueInstance(7-7)
⏰ 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 (9)
app/javascript/components/activity/ProductTotals.vue (1)
80-80: Good use of template literals.The template literal syntax is cleaner and more maintainable than string concatenation.
app/javascript/users.js (3)
5-5: LGTM!The module-scoped
vueInstancevariable follows the consistent pattern used across other entrypoints (credit_mutations.js, invoices.js, activity.js).
7-12: LGTM!Proper cleanup of the Vue instance before Turbo caches the page prevents memory leaks and stale state issues during navigation.
14-18: LGTM! Previous critical issue resolved.The CSRF token setup now includes proper null checking, addressing the previous review's concern about potential
TypeErrorwhen the meta tag is missing.app/javascript/price_lists.js (2)
142-156: Correct VueResource usage in archive/unarchive methods.These methods correctly use VueResource patterns:
response.body(lines 144, 152) anderror.response?.body?.errors(lines 146, 154). This is the correct approach for VueResource, unlike the inconsistent patterns insaveProductabove.
77-88: Incorrect response property access for VueResource (line 81).Line 81 accesses
response.dataafter athis.$http.postcall, but VueResource usesresponse.bodyfor the response payload. Note that line 86 correctly useserror.response?.body?.errorsfor the error handler, making the pattern inconsistent within the same function.Apply this diff:
this.$http.post('/products.json', { product: sanitizedProduct }).then( (response) => { const index = this.products.indexOf(product); this.products.splice(index, 1); - const newProduct = response.data; + const newProduct = response.body; newProduct.editing = false; this.products.push(newProduct); }).catch((error) => { this.errors = error.response?.body?.errors || ['An error occurred']; });Likely an incorrect or invalid review comment.
app/javascript/activities.js (3)
1-7: VueResource usage in a migration PR.This file imports and uses VueResource (lines 2, 4, 7), which is consistent internally but raises the question of whether VueResource should be retained across the codebase. The PR objectives mention migrating to modern Rails 7 tooling, and past review comments on
price_lists.jssuggested moving to axios. Consider whether a codebase-wide decision has been made about VueResource vs. axios for HTTP requests.That said, the code itself is correct: VueResource is properly imported, configured, and the CSRF token setup is appropriate.
6-10: Clean Turbo integration and element guard.The migration from
turbolinks:loadtoturbo:load(line 6) is correct, and the element existence guard (line 9) prevents errors when the modal isn't present on the page. Well done.
28-28: Improved case-insensitive filtering.The case-insensitive filtering using
.toLowerCase()on both the value name and query is a good UX improvement.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/javascript/components/activity/ProductTotals.vue(1 hunks)app/javascript/order_screen.js(8 hunks)app/javascript/price_lists.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/javascript/components/activity/ProductTotals.vue
- app/javascript/price_lists.js
⏰ 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 (8)
app/javascript/order_screen.js (8)
1-9: LGTM: Imports properly configured for the new pipeline.The imports correctly reference local components and include both VueResource and axios. This aligns with the migration to js-bundling and addresses the previous review feedback about missing axios.
11-31: LGTM: Turbo lifecycle integration properly implemented.The migration from
turbolinks:loadtoturbo:loadis correct, and CSRF tokens are appropriately configured for both VueResource and axios. The dataset parsing uses const declarations, which is good practice.
65-75: LGTM: User refresh logic correctly uses findIndex.The updated logic properly uses
findIndexwith ID comparison to locate and update users, handling both existing and new user scenarios. This correctly addresses the previous review feedback.
176-181: LGTM: Order confirmation user update follows the correct pattern.Uses
findIndexwith ID comparison consistently with the rest of the file.
228-240: LGTM: SumUp integration with corrected property name.The typo fix from
affilateKeytoaffiliateKeyis correct, and the platform-specific URL construction logic is properly maintained.
332-348: LGTM: Validation logic properly extracted into a helper method.The
isFormInvalid()helper consolidates validation checks and improves code organization. Its usage insaveCreditMutation()is correct.
357-367: LGTM: Credit mutation user update uses the correct pattern.Consistently applies
findIndexwith ID comparison to update the user in the array.
431-431: LGTM: Event listener properly closed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/javascript/order_screen.js (1)
359-377: Fix indentation aroundsetTimeoutto satisfy ESLint.The logic for updating
app.usersafter a credit mutation and hiding the modal is good, but ESLint is complaining about the indentation on thesetTimeoutblock (lines 372–373). Adjust the inner line and closing line so they are indented relative to the callback and the surrounding method:- /* eslint-disable no-undef */ - setTimeout(() => { - bootstrap.Modal.getOrCreateInstance('#credit-mutation-modal').hide(); - }, 0); + /* eslint-disable no-undef */ + setTimeout(() => { + bootstrap.Modal.getOrCreateInstance('#credit-mutation-modal').hide(); + }, 0);This should clear the reported
indenterrors while preserving behavior.
♻️ Duplicate comments (1)
app/javascript/order_screen.js (1)
205-221: Fix VueResource error handling: useerror.status/error.bodyinstead oferror.response.
handleXHRErrortreats its argument as{ response: { status, body }}but VueResource passes the response object directly, soerror.response?.statusanderror.response.bodyare alwaysundefined. This breaks all status‑based handling and Sentry logging; every error will fall through to"Error undefined?!🤔".Update the method to read from the response object itself:
- handleXHRError(error) { - if (error.response?.status === 500) { + handleXHRError(error) { + if (error.status === 500) { this.sendFlash('Server error!', 'Herlaad de pagina', 'error'); try { - throw new Error(JSON.stringify(error.response.body)); + throw new Error(JSON.stringify(error.body)); } catch(e) { /* eslint-disable no-undef */ Sentry.captureException(e); /* eslint-enable no-undef */ } - } else if (error.response?.status === 422) { + } else if (error.status === 422) { this.sendFlash('Error bij het opslaan!', 'Probeer het opnieuw', 'warning'); } else { - this.sendFlash(`Error ${error.response?.status}?!🤔`, 'Herlaad de pagina', 'info'); + this.sendFlash(`Error ${error.status}?!🤔`, 'Herlaad de pagina', 'info'); } },In VueResource 1.5.x, what does the error callback of `this.$http.post(...).then(success, error)` receive—does it get a `Response` object with `.status` and `.body` directly, or an object with a nested `.response` property?
🧹 Nitpick comments (1)
app/javascript/order_screen.js (1)
163-193: Post‑order user update is correct; consider filtering out empty order rows.Updating the
usersarray viafindIndexonuser.idand$setwithresponse.body.useris sound and avoids the earlierindexOfbrittleness. One small improvement while you’re in here:order_rows_attributesbuilt viamapwill containundefinedentries for rows with falsyamount. Consider filtering them out (e.g.,.filter(Boolean)or building the array withreduce) so you don’t send null entries to the backend.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/javascript/order_screen.js(9 hunks)app/views/activities/_credit_mutation_modal.html.erb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/activities/_credit_mutation_modal.html.erb
🧰 Additional context used
🪛 ESLint
app/javascript/order_screen.js
[error] 372-372: Expected indentation of 14 spaces but found 12.
(indent)
[error] 373-373: Expected indentation of 12 spaces but found 10.
(indent)
⏰ 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 (5)
app/javascript/order_screen.js (5)
4-21: Turbo load init, imports, and dataset parsing look consistent.The added
bootstrapimport, switch toturbo:load, and the way you parse datasets intoconstvalues and seed Vue state are coherent and don't introduce any obvious issues.
63-85: User refresh logic correctly syncsusersandselectedUser.Using
response.bodyplusfindIndexonidand then updating or appendingrefreshedUser(and reassigningselectedUserwhen IDs match) makes the user state consistent after a reload without breaking existing behavior.
229-241: SumUp URL construction and dataset usage look fine.Reading
sumupKey/sumupCallbackfromelement.datasetand composing the SumUp URL with iOS vs. other handling is straightforward and consistent; no functional issues stand out here.
333-347:isFormInvalidhelper cleanly centralizes credit mutation validation.The helper’s combination of
checkValidity()with explicit checks for user, amount, and description, and wiring it intosaveCreditMutationviacreditMutationFormInvalid, makes the validation logic clearer without changing behavior.
433-434: Outer Turbo load handler closes correctly.The final closing braces and parenthesis for the
document.addEventListener('turbo:load', ...)wrapper are aligned and consistent; nothing else needed here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/javascript/order_screen.js (1)
205-221: Fix VueResource error response property access.The error handling accesses
error.response?.statusanderror.response.body, but VueResource error callbacks receive a response object withstatusandbodyproperties directly—not nested under a.responseproperty. This breaks all error handling: the status checks will always fail becauseerror.response?.statusreturnsundefined, causing every error to fall through to the else case.Apply this diff to fix the property access:
handleXHRError(error) { - if (error.response?.status === 500) { + if (error.status === 500) { this.sendFlash('Server error!', 'Herlaad de pagina', 'error'); try { - throw new Error(JSON.stringify(error.response.body)); + throw new Error(JSON.stringify(error.body)); } catch(e) { /* eslint-disable no-undef */ Sentry.captureException(e); /* eslint-enable no-undef */ } - } else if (error.response?.status === 422) { + } else if (error.status === 422) { this.sendFlash('Error bij het opslaan!', 'Probeer het opnieuw', 'warning'); } else { - this.sendFlash(`Error ${error.response?.status}?!🤔`, 'Herlaad de pagina', 'info'); + this.sendFlash(`Error ${error.status}?!🤔`, 'Herlaad de pagina', 'info'); } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/javascript/order_screen.js(9 hunks)
🧰 Additional context used
🪛 ESLint
app/javascript/order_screen.js
[error] 371-371: Expected indentation of 12 spaces but found 10.
(indent)
[error] 372-372: Expected indentation of 14 spaces but found 12.
(indent)
[error] 373-373: Expected indentation of 12 spaces but found 10.
(indent)
⏰ 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 (6)
app/javascript/order_screen.js (6)
1-21: LGTM! Clean migration to Turbo lifecycle.The imports, CSRF token setup, and migration from
turbolinks:loadtoturbo:loadare all correct. The data parsing properly usesconstfor immutable references.
66-76: LGTM! Robust user refresh logic.The use of
findIndexwith ID comparison correctly handles both updating existing users and appending new ones. TheselectedUsersynchronization ensures consistency.
177-182: LGTM! Consistent user update pattern.The order success handler correctly updates the user entry using
findIndexwith ID comparison, matching the pattern used elsewhere in the file.
229-241: LGTM! SumUp integration looks solid.The payment URL construction correctly reads from
dataset, handles iOS vs. other platforms appropriately, and builds the proper SumUp merchant URL.
333-339: LGTM! Good validation refactor.Extracting the validation logic into
isFormInvalid()improves readability and maintainability. The combination of native form validation with custom presence checks is appropriate.
358-368: LGTM! Credit mutation user handling is consistent.The user array update correctly uses
findIndexwith ID comparison, and the conditional user selection logic appropriately handles thekeepUserSelectedflag and empty order rows.
| setTimeout(() => { | ||
| bootstrap.Modal.getOrCreateInstance('#credit-mutation-modal').hide(); | ||
| }, 0); |
There was a problem hiding this comment.
Fix indentation to resolve ESLint failure.
Lines 371-373 have incorrect indentation, causing the ESLint check to fail and blocking the pipeline.
Apply this diff:
/* eslint-disable no-undef */
- setTimeout(() => {
+ setTimeout(() => {
- bootstrap.Modal.getOrCreateInstance('#credit-mutation-modal').hide();
+ bootstrap.Modal.getOrCreateInstance('#credit-mutation-modal').hide();
- }, 0);
+ }, 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setTimeout(() => { | |
| bootstrap.Modal.getOrCreateInstance('#credit-mutation-modal').hide(); | |
| }, 0); | |
| setTimeout(() => { | |
| bootstrap.Modal.getOrCreateInstance('#credit-mutation-modal').hide(); | |
| }, 0); |
🧰 Tools
🪛 ESLint
[error] 371-371: Expected indentation of 12 spaces but found 10.
(indent)
[error] 372-372: Expected indentation of 14 spaces but found 12.
(indent)
[error] 373-373: Expected indentation of 12 spaces but found 10.
(indent)
🤖 Prompt for AI Agents
In app/javascript/order_screen.js around lines 371 to 373, the setTimeout
callback and its contents are mis-indented causing an ESLint failure; adjust
indentation so the setTimeout line and its inner call are aligned with
surrounding code (use two or four spaces consistent with project style) and
ensure the closing brace and parentheses match the original indentation level to
satisfy ESLint.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/javascript/order_screen.js (1)
358-359: UsefindIndexwith ID comparison instead ofindexOf.Line 359 uses
indexOfwhich was flagged in previous reviews as brittle—ifselectedUseris a refreshed object,indexOfwill fail to locate it. UsefindIndexwith ID comparison for robustness, consistent with the pattern used elsewhere in this file (lines 67, 178).Apply this diff:
}).then((response) => { - app.$set(app.users, app.users.indexOf(app.selectedUser), response.body.user); + const index = app.users.findIndex((candidate) => candidate.id === response.body.user.id); + if (index !== -1) { + app.$set(app.users, index, response.body.user); + } if(!app.keepUserSelected && app.orderRows.length === 0){
♻️ Duplicate comments (1)
app/javascript/order_screen.js (1)
205-221: Fix VueResource error response property access.This critical issue was flagged in previous reviews but remains unaddressed. VueResource error callbacks receive the response object directly—the
statusandbodyproperties are not nested under.response. With the current code,error.response?.statusreturnsundefined, causing all errors to show "Error undefined?!🤔" instead of the appropriate message.Apply this diff to fix the property access:
handleXHRError(error) { - if (error.response?.status === 500) { + if (error.status === 500) { this.sendFlash('Server error!', 'Herlaad de pagina', 'error'); try { - throw new Error(JSON.stringify(error.response.body)); + throw new Error(JSON.stringify(error.body)); } catch(e) { /* eslint-disable no-undef */ Sentry.captureException(e); /* eslint-enable no-undef */ } - } else if (error.response?.status === 422) { + } else if (error.status === 422) { this.sendFlash('Error bij het opslaan!', 'Probeer het opnieuw', 'warning'); } else { - this.sendFlash(`Error ${error.response?.status}?!🤔`, 'Herlaad de pagina', 'info'); + this.sendFlash(`Error ${error.status}?!🤔`, 'Herlaad de pagina', 'info'); } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/javascript/order_screen.js(8 hunks)
🧰 Additional context used
🪛 ESLint
app/javascript/order_screen.js
[error] 368-368: Expected indentation of 12 spaces but found 10.
(indent)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/javascript/order_screen.js (2)
336-373: Fix credit mutation user update robustness and ESLint indentation.Two issues in this block:
Brittle
indexOfwithout guard:
app.$set(app.users, app.users.indexOf(app.selectedUser), response.body.user);assumesselectedUseris always present inusers.- If it’s ever missing (e.g., list reloaded independently),
indexOfreturns-1andVue.setwill mutate the wrong entry.- Prefer an
id-basedfindIndexwith anindex !== -1guard, consistent with the order-confirmation flow.Indentation breaks ESLint (line 361):
- The
bootstrap.Modal.getOrCreateInstance(...).hide();line is under-indented relative to the surrounding block, matching the reported ESLint error.- Adjusting indentation by two spaces will resolve the lint failure.
Apply this diff:
}).then((response) => { - app.$set(app.users, app.users.indexOf(app.selectedUser), response.body.user); + const index = app.users.findIndex( + (candidate) => candidate.id === response.body.user.id + ); + if (index !== -1) { + app.$set(app.users, index, response.body.user); + } @@ - /* eslint-disable no-undef */ - bootstrap.Modal.getOrCreateInstance('#credit-mutation-modal').hide(); + /* eslint-disable no-undef */ + bootstrap.Modal.getOrCreateInstance('#credit-mutation-modal').hide();(Indentation assumes 2 extra spaces to align with other statements in this
.thenblock.)
205-221: VueResource error handling still reads fromerror.response, breaking all status checks.For VueResource, the rejected promise handler receives the response object directly (
status,body, …), not nested undererror.response. As written,error.response?.statusis alwaysundefined, so:
- 500/422 branches are never taken,
- the fallback message shows
Error undefined?!🤔,- the Sentry payload uses
error.response.body, which is alsoundefined.Switch to
error.statusanderror.bodyso the handler actually reflects the HTTP response.Apply this diff:
- handleXHRError(error) { - if (error.response?.status === 500) { + handleXHRError(error) { + if (error.status === 500) { this.sendFlash('Server error!', 'Herlaad de pagina', 'error'); try { - throw new Error(JSON.stringify(error.response.body)); + throw new Error(JSON.stringify(error.body)); } catch(e) { /* eslint-disable no-undef */ Sentry.captureException(e); /* eslint-enable no-undef */ } - } else if (error.response?.status === 422) { + } else if (error.status === 422) { this.sendFlash('Error bij het opslaan!', 'Probeer het opnieuw', 'warning'); } else { - this.sendFlash(`Error ${error.response?.status}?!🤔`, 'Herlaad de pagina', 'info'); + this.sendFlash(`Error ${error.status}?!🤔`, 'Herlaad de pagina', 'info'); } },If you have customized
this.$httpaway from VueResource semantics, please double-check the response shape in your docs or devtools.What is the shape of the error object passed to VueResource's promise rejection handler, and does it expose `status` and `body` directly?
🧹 Nitpick comments (2)
app/views/activities/_credit_mutation_modal.html.erb (2)
19-20: Preserve accessibility semantics for the currency iconSwitching from the
fa_iconhelper to a raw<i>likely dropped defaultaria-hiddenbehavior. For a purely decorative currency icon inside an input group, it’s better to hide it from screen readers.- <div class="input-group-text"> - <i class="fas fa-euro-sign"></i> - </div> + <div class="input-group-text"> + <i class="fas fa-euro-sign" aria-hidden="true"></i> + </div>
62-64: Confirm UX when auto‑dismissing the modal on save clickAdding
data-bs-dismiss="modal"means the modal will close immediately on click, regardless of whatsaveCreditMutationdoes. If that method performs async work or can fail/validate client-side, users may never see errors or get a chance to correct input before the dialog disappears.Also note that this button is outside the
<form>, sotype="submit"will not trigger native form submission/validation unless you’re handling it manually elsewhere.Consider either:
- Letting
saveCreditMutationcontrol when to close the modal (removedata-bs-dismiss="modal"and programmatically hide the modal on success), and/or- Associating the button with the form (moving it inside the
<form>or addingform="credit-mutation-modal-form") if you want native validation semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/javascript/components/UserInput.vue(3 hunks)app/javascript/credit_mutations.js(2 hunks)app/javascript/order_screen.js(8 hunks)app/views/activities/_credit_mutation_modal.html.erb(2 hunks)
🧰 Additional context used
🪛 ESLint
app/javascript/order_screen.js
[error] 361-361: Expected indentation of 12 spaces but found 10.
(indent)
⏰ 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 (7)
app/javascript/order_screen.js (4)
4-21: Turbo bootstrap setup and data extraction look solid.Importing Bootstrap as a module and wiring the
turbo:loadlistener with CSRF header setup and JSON dataset parsing is consistent and correct for the new js-bundling/Turbo pipeline; no changes needed here.
63-76: User refresh logic onsetUseris robust.Refreshing the user via
this.$http.get, updating theusersarray byid, pushing missing users, and re-pointingselectedUserwhen ids match correctly keeps local state in sync with the server while avoiding stale selections or race conditions.
177-181: Order confirmation correctly updates the user entry.Using
findIndexonthis.userswith anidcomparison plus anindex !== -1guard is a safe way to update the user after an order; this avoids the brittleindexOf(selectedUser)pattern and prevents accidental writes when the user is missing.
229-234: SumUp URL construction and affiliate key rename look correct.Using
affiliate-key=${affiliateKey}withaffiliateKeysourced fromelement.dataset.sumupKeyand splitting iOS vs. other platforms (amountvstotal, dedicated success/fail callbacks) preserves the previous behavior while adopting the new naming; this looks good.app/javascript/components/UserInput.vue (2)
46-71: Debounced input handling and empty-query reset look good.The
debounceTimerpattern (clearTimeouton eachupdateValuecall,setTimeoutforupdateSuggestions) plus the early-return path that clearsselectedSuggestionandsuggestionswhenquery === ''is clean and should avoid both flicker and stale results.
94-114: Suggestion click handling and debounce cleanup are correct.
suggestionClickedcorrectly syncsselectedSuggestion,query, dropdown state, and emits the selection, andbeforeDestroyclearingdebounceTimerprevents strayupdateSuggestionsexecutions after component teardown.app/javascript/credit_mutations.js (1)
4-20: Credit mutations entrypoint aligns correctly with Turbo and new component path.Switching to
turbo:load, importingUserInputfrom./components/UserInput.vue, and usingconst elementwhile guarding onelement != nullis all consistent with the new asset structure and VueResource setup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/javascript/order_screen.js (2)
176-180: Consider adding defensive push for consistency.The user update after order placement only updates if found, while the pattern at lines 68-72 (in
setUser) also pushes if not found. For consistency and defensive programming, consider adding anelsebranch:if (user) { const index = this.users.findIndex((candidate) => candidate.id === user.id); if (index !== -1) { this.$set(this.users, index, response.body.user); + } else { + this.users.push(response.body.user); } }In practice, the user should already be in the array when placing orders, but this ensures robustness if assumptions change.
349-353: Consider adding defensive push for consistency.Similar to the order confirmation flow, this credit mutation save only updates the user if found. For consistency with the pattern at lines 68-72, consider handling the not-found case:
const index = app.users.findIndex((candidate) => candidate.id === response.body.user.id); if (index !== -1) { app.$set(app.users, index, response.body.user); +} else { + app.users.push(response.body.user); }This ensures the users array remains consistent even if assumptions about pre-existing users change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitattributes(1 hunks)app/javascript/order_screen.js(8 hunks)spec/controllers/orders_controller/update_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitattributes
⏰ 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). (2)
- GitHub Check: Lint
- GitHub Check: Test
🔇 Additional comments (5)
spec/controllers/orders_controller/update_spec.rb (1)
14-21: Frontend code correctly updated to match new nested parameter structure.Verification confirms that the frontend code in
app/javascript/components/ProductTable.vue(line 88) correctly sends PATCH requests with the nested parameter structure:axios.patch(/orders/${id}, { order: { order_rows_attributes: [...] } }). This matches the expected parameter format in the test file and is consistent with both POST requests (inorder_screen.js) and PATCH requests for order updates. The breaking change to the API contract has been properly implemented across the frontend.app/javascript/order_screen.js (4)
12-21: LGTM! Turbo migration and data extraction.The migration from
turbolinks:loadtoturbo:loadis correct, and the data extraction pattern using const with proper null checks is solid.
66-76: LGTM! Robust user refresh logic.The pattern of finding by ID and pushing if not found ensures the users array stays consistent after reload. This defensive approach is good practice.
204-220: LGTM! Error handling correctly accesses VueResource properties.The error handling now correctly uses
error.statusanderror.bodyfor VueResource error responses, with appropriate user feedback and Sentry logging for server errors.
228-240: LGTM! SumUp integration with correct variable naming.The
affiliateKeyvariable naming is now consistent with JavaScript conventions, and the iOS vs non-iOS callback handling is clear.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/javascript/components/UserInput.vue (2)
73-90: Suggestion list building is safer; consider stable keys for synthetic options.Initializing
resultsand assigningthis.suggestionsonce is a good improvement and avoids transient inconsistent state. One minor follow‑up: the “Gepind” / “Contant betaald” entries still have noidwhile the template uses:key="suggestion.id", which can lead toundefined/duplicate keys when these appear.If it’s safe for the backend, you could give these entries a stable
id:- if (this.includePin && 'gepind'.indexOf(this.query.toLowerCase()) >= 0) { - results.push({ name: 'Gepind', paid_with_pin: true }); - } + if (this.includePin && 'gepind'.indexOf(this.query.toLowerCase()) >= 0) { + results.push({ id: '__pin', 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 }); - } + if (this.includeCash && 'contant betaald'.indexOf(this.query.toLowerCase()) >= 0) { + results.push({ id: '__cash', name: 'Contant betaald', paid_with_cash: true }); + }Alternatively, you can keep the payloads as‑is and adjust the
:keyto fall back tosuggestion.nameoutside this hunk.
94-100:suggestionClickedflow is fine; comments could be tightened.The click/enter path correctly updates
selectedSuggestion, syncsquery, closes the dropdown, and emits the value. The inline comments like “this logic is still correct” don’t add much and may become misleading over time; consider either removing them or replacing them with a short domain‑level note (e.g. what the parent can expect from the emitted payload).Also applies to: 102-102
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/javascript/components/UserInput.vue(3 hunks)app/javascript/order_screen.js(8 hunks)
⏰ 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 (7)
app/javascript/components/UserInput.vue (1)
46-48: Debounce implementation and cleanup look solid.Clearing the timeout on each input and again in
beforeDestroyavoids overlapping calls and stray callbacks; keepingdebounceTimerinternal to the component keeps the public API unchanged.Also applies to: 59-60, 64-65, 68-71, 111-116
app/javascript/order_screen.js (6)
1-8: LGTM! Imports properly updated for the new bundler.The imports have been correctly migrated: axios is now imported, bootstrap is available, and component paths use the appropriate relative prefixes for js-bundling.
12-21: LGTM! Clean Turbo migration and improved variable declarations.The event listener has been correctly updated from
turbolinks:loadtoturbo:load, and the data initialization now properly usesconstdeclarations for immutability.
66-76: LGTM! Robust user refresh logic.The user refresh flow correctly uses
findIndexwith ID comparison to update existing users or append new ones, and properly synchronizesselectedUserwhen the refreshed data matches.
206-222: LGTM! Error handling correctly updated for VueResource.The error handling now properly accesses
error.statusanderror.bodydirectly, which is correct for VueResource error callbacks.
230-242: LGTM! SumUp integration variable naming corrected.The typo has been fixed from
affilateKeytoaffiliateKey, and the URL construction logic correctly uses the updated variable name.
351-357: LGTM! Consistent user update pattern.The credit mutation handler correctly uses the same robust
findIndexpattern to update or append the user, maintaining consistency across the codebase.
2 participants
Remove the old way of serving javascript through rails and replaced it with a rails 7 alternative.
This was needed because it was base on webpack 4 and we needed to move to webpack 5. Webpack 4 is already 3 years old.
The goal was to get all assest handeld by js-bundeling rails this is the reason why we also switched the way of serving icons
Besides that because i was overhauling the javascript pipeline. I could do some needed migrations
Fixes #976
Fixes #977
Fixes #978
Fixes #979
Fixes #688
5 errors
TO DO
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.