Update/migrate webpacker to js bundeling #1108
Conversation
WalkthroughMigrates front-end from Webpacker/Turbolinks to a Rails 7+ bundling setup: adds Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant Rails as Rails (layout)
participant AppJS as application.js
participant PageJS as Page module
participant Vue as Vue instance
Browser->>Rails: GET page
Rails->>Browser: HTML (head includes + application.js defer + per-page script)
Browser->>AppJS: execute deferred `application.js`
AppJS->>AppJS: init Turbo, WebFont, axios, CSRF headers
Note right of AppJS: global setup (fonts, headers)
Browser->>PageJS: load per-page JS (deferred)
PageJS->>PageJS: subscribe to `turbo:load`
AppJS->>PageJS: `turbo:load` fired (navigation complete)
PageJS->>PageJS: read element.dataset, prepare props
PageJS->>Vue: mount component
Vue->>Browser: DOM interactions & user events
Browser->>PageJS: navigation away (caching)
PageJS->>Vue: destroy on `turbo:before-cache`
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Focus areas:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config/environments/development.rb (1)
22-22: Bug: wrong caching toggle path prevents dev caching switch.
Should be tmp/caching-dev.txt (single segment), not nested twice.- if Rails.root.join('tmp', 'caching-dev.txt', 'caching-dev.txt').exist? + if Rails.root.join('tmp', 'caching-dev.txt').exist?app/javascript/order_screen.js (1)
214-218: Fix typo in variable name.Line 215:
affilateKeyshould beaffiliateKey.startSumupPayment(orderId, orderTotal) { - let affilateKey = element.dataset.sumupKey; + let affiliateKey = element.dataset.sumupKey; let callback = element.dataset.sumupCallback; - let sumupUrl = `sumupmerchant://pay/1.0?affiliate-key=${affilateKey}¤cy=EUR&title=Bestelling ${element.dataset.siteName}&skip-screen-success=true&foreign-tx-id=${orderId}`; + let sumupUrl = `sumupmerchant://pay/1.0?affiliate-key=${affiliateKey}¤cy=EUR&title=Bestelling ${element.dataset.siteName}&skip-screen-success=true&foreign-tx-id=${orderId}`;
🧹 Nitpick comments (16)
.gitignore (2)
46-51: Deduplicate ignore entries.
The entries for /public/packs, /public/packs-test, /node_modules, yarn logs, and .yarn-integrity appear twice. Remove the second block to reduce churn.- /public/packs - /public/packs-test - /node_modules - /yarn-error.log - yarn-debug.log* - .yarn-integrity
57-57: Scoped ignore for gemini.md.
If this is a local notes file, consider moving it under /docs or naming it more specifically (e.g., /docs/gemini.md) to avoid accidental clashes.README.md (2)
8-8: Grammar tweak: “built-in”.
Change “builtin” → “built-in”. Short and clearer. Based on static analysis hints.-The source code belonging to Alpha SOFIA. It is a system built with Ruby on Rails with Turbo and a little VueJS, used to manage orders in our own bar "Flux". Users authenticate via OAuth API (currently "Alpha AMBER") or through the builtin login portal to see how much credit they got left, or to be able to register new orders and/or payments. +The source code belonging to Alpha SOFIA. It is a system built with Ruby on Rails with Turbo and a little VueJS, used to manage orders in our own bar "Flux". Users authenticate via OAuth API (currently "Alpha AMBER") or through the built-in login portal to see how much credit they have left, or to register new orders and/or payments.
24-31: Update dev run steps for jsbundling (watcher).
With jsbundling/webpack, add either bin/dev (Procfile.dev/foreman) or a parallel yarn build --watch step; otherwise JS won’t rebuild during development.Example (if bin/dev exists):
- Run: bin/dev
Or add:- In one terminal: yarn build --watch
- In another: bundle exec rails s -b 0.0.0.0 -p 5000
app/javascript/activity.js (2)
15-19: Import Bootstrap’s Tab module instead of relying on a global.
Avoids runtime no-undef and trims bundle.+import Tab from 'bootstrap/js/dist/tab'; @@ - /* eslint-disable no-undef */ - const firstTab = new bootstrap.Tab(firstTabEl); + const firstTab = new Tab(firstTabEl); firstTab.show();
5-13: Tear down Vue on Turbo cache to prevent memory leaks.
Destroy the instance on turbo:before-cache; otherwise observers may linger.document.addEventListener('turbo:load', () => { const element = document.getElementById('activity'); if (element) { - new Vue({ + const app = new Vue({ el: element, components: { ProductTotals } }); + + document.addEventListener('turbo:before-cache', () => { + app.$destroy(); + }, { once: true });.babelrc (1)
18-34: Improve test transpilation reliability.
Explicitly set modules: "commonjs" in env.test to help Node/Jest."env": { "test": { - "presets": [ + "presets": [ [ "@babel/preset-env", { - "targets": { - "node": "current" - } + "targets": { "node": "current" }, + "modules": "commonjs" } ] ],app/javascript/application.js (1)
23-27: Consider moving WebFont loading outside the turbo:load listener.The
WebFont.load()call will execute on every Turbo navigation. Since fonts only need to be loaded once per session, consider moving this outside the event listener or wrapping it with a guard to prevent redundant font loading requests.Apply this diff to optimize font loading:
+WebFont.load({ + google: { + families: ['Roboto:300,400,500'] + } +}); + document.addEventListener('turbo:load', () => { const csrfToken = document.querySelector('meta[name="csrf-token"]')?.getAttribute('content'); if (csrfToken) { Vue.http.headers.common['X-CSRF-TOKEN'] = csrfToken; axios.defaults.headers.common['X-CSRF-Token'] = csrfToken; } - - /* eslint-disable no-undef */ - WebFont.load({ - google: { - families: ['Roboto:300,400,500'] - } - }); - /* eslint-enable no-undef */ });app/javascript/price_lists.js (1)
40-48: Inconsistent return value from newProduct method.The method returns
this.products.push(newProduct), which returns the new array length (a number) rather than the product or array. This is inconsistent with the pattern in other methods and may not be the intended behavior.Consider one of these approaches:
Option 1: Return the new product
newProduct () { const newProduct = { name: '', category: 0, editing: true, product_prices: [], }; - return this.products.push(newProduct); + this.products.push(newProduct); + return newProduct; },Option 2: Return nothing (if unused)
newProduct () { const newProduct = { name: '', category: 0, editing: true, product_prices: [], }; - return this.products.push(newProduct); + this.products.push(newProduct); },app/javascript/invoices.js (1)
46-70: Consider using element-specific DOM methods for more robust row cloning.The current implementation uses
lastChildandchildNodes, which include text nodes and comments. This could lead to unexpected behavior if the HTML structure changes.Apply this diff for more robust element handling:
addRow() { const invoiceRowContainer = document.getElementById('invoice_row'); - const lastRow = invoiceRowContainer.lastChild; + const lastRow = invoiceRowContainer.lastElementChild; const newRow = lastRow.cloneNode(true); invoiceRowContainer.appendChild(newRow); - newRow.childNodes.forEach((fieldWrapper) => { + Array.from(newRow.children).forEach((fieldWrapper) => { let newIndex = -1; - if (fieldWrapper.nodeType === Node.ELEMENT_NODE && fieldWrapper.firstChild) { - const nameAttr = fieldWrapper.firstChild.name; + if (fieldWrapper.firstElementChild) { + const nameAttr = fieldWrapper.firstElementChild.name; if (nameAttr) { if (newIndex === -1) { const match = nameAttr.match(/invoice\[rows_attributes\]\[(\d+)\]/); if (match && match[1]) { newIndex = Number(match[1]) + 1; } } if (newIndex !== -1) { - fieldWrapper.firstChild.name = nameAttr.replace(/\[\d+\]/g, '[' + newIndex + ']'); - fieldWrapper.firstChild.value = ''; + fieldWrapper.firstElementChild.name = nameAttr.replace(/\[\d+\]/g, '[' + newIndex + ']'); + fieldWrapper.firstElementChild.value = ''; } } } }); }webpack.config.js (1)
20-20: Consider environment-aware source map configuration.Source maps are currently generated for all builds. While helpful for debugging, exposing full source maps in production can leak proprietary code and increase bundle size.
Apply this diff for environment-aware source maps:
- devtool: "source-map", + devtool: process.env.NODE_ENV === "production" ? "hidden-source-map" : "source-map",This generates source maps in production for error tracking (uploaded to Sentry) but doesn't expose them to end users.
bin/dev (1)
3-6: Consider handling gem installation permissions.The
gem install foremancommand may require elevated permissions on some systems. Consider suggestingbundle add foreman --group developmentin the error message as an alternative.Optional improvement:
if gem list --no-installed --exact --silent foreman; then - echo "Installing foreman..." - gem install foreman + echo "Foreman is not installed. Installing..." + gem install foreman || { + echo "Failed to install foreman globally. Try: bundle add foreman --group development" + exit 1 + } fiapp/javascript/users.js (1)
4-23: Consider adding Vue instance cleanup for Turbo caching.The Vue instance is created on every
turbo:loadevent but is never explicitly destroyed. When Turbo caches pages for back/forward navigation, the old Vue instance may remain in memory, potentially causing memory leaks or duplicate event handlers.Consider storing a reference to the Vue instance and destroying it before the page is cached:
+let vueInstance = null; + +document.addEventListener('turbo:before-cache', () => { + if (vueInstance) { + vueInstance.$destroy(); + vueInstance = null; + } +}); + document.addEventListener('turbo:load', () => { const element = document.getElementById('users-index'); if (element !== null) { const manual_users = JSON.parse(element.dataset.manualUsers); const amber_users = JSON.parse(element.dataset.amberUsers); const inactive_users = JSON.parse(element.dataset.inactiveUsers); - new Vue({ + vueInstance = new Vue({ el: element, data: () => ({ manual_users, amber_users, inactive_users }), components: { UsersTable }, }); } });app/javascript/credit_mutations.js (1)
5-16: Consider adding Vue instance cleanup for Turbo caching.Similar to other Vue initialization files in this PR, the Vue instance should be destroyed before Turbo caches the page to prevent memory leaks.
Apply the same pattern recommended for
app/javascript/users.js:+let vueInstance = null; + +document.addEventListener('turbo:before-cache', () => { + if (vueInstance) { + vueInstance.$destroy(); + vueInstance = null; + } +}); + document.addEventListener('turbo:load', () => { - const element = document.getElementById('new_mutation_modal'); if (element != null) { - new Vue({ + vueInstance = new Vue({ el: element, components: { UserInput } }); } });app/javascript/payment_add.js (1)
19-23: Consider extracting the minimum payment amount to a constant.The minimum payment value
20appears in both the default value (line 11) and validation logic (line 21). Extracting it to a constant would improve maintainability.+const MIN_PAYMENT_AMOUNT = 20; + document.addEventListener('turbo:load', () => { const element = document.getElementById('payment-add'); if (element !== null) { new Vue({ el: element, data: () => { return { currentCredit: parseFloat(element.dataset.userCredit) || 0, - paymentAmount: parseFloat(element.dataset.paymentAmount) || 20 + paymentAmount: parseFloat(element.dataset.paymentAmount) || MIN_PAYMENT_AMOUNT }; }, computed: { newCredit() { return (this.currentCredit + this.paymentAmount).toFixed(2); } }, methods: { amountValid() { - return this.paymentAmount >= 20; + return this.paymentAmount >= MIN_PAYMENT_AMOUNT; } }, }); } });app/javascript/order_screen.js (1)
321-322: Consider extracting complex validation into a helper method.The validation logic on lines 321-322 is quite dense and could be more readable if extracted into a separate method or broken into intermediate variables.
methods: { + isFormInvalid() { + const formValid = document.getElementById('credit-mutation-modal-form').checkValidity(); + const hasUser = !!app.selectedUser; + const hasAmount = !!this.creditMutationAmount; + const hasDescription = !!this.creditMutationDescription; + return !formValid || !hasUser || !hasAmount || !hasDescription; + }, + saveCreditMutation() { this.isSubmitting = true; - this.creditMutationFormInvalid = (!document.getElementById('credit-mutation-modal-form').checkValidity() - || !app.selectedUser || !this.creditMutationAmount || !this.creditMutationDescription); + this.creditMutationFormInvalid = this.isFormInvalid();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (54)
.babelrc(1 hunks).gitignore(2 hunks)Gemfile(2 hunks)Procfile.dev(1 hunks)README.md(1 hunks)app/assets/config/manifest.js(1 hunks)app/assets/javascripts/application.js(0 hunks)app/assets/stylesheets/application.scss(1 hunks)app/assets/stylesheets/order_screen.scss(1 hunks)app/javascript/activities.js(3 hunks)app/javascript/activity.js(1 hunks)app/javascript/application.js(1 hunks)app/javascript/credit_mutations.js(1 hunks)app/javascript/invoices.js(1 hunks)app/javascript/order_screen.js(2 hunks)app/javascript/packs/activity.js(0 hunks)app/javascript/packs/credit_mutations.js(0 hunks)app/javascript/packs/invoices.js(0 hunks)app/javascript/packs/payment_add.js(0 hunks)app/javascript/packs/user.js(0 hunks)app/javascript/packs/users.js(0 hunks)app/javascript/payment_add.js(1 hunks)app/javascript/price_lists.js(8 hunks)app/javascript/user.js(1 hunks)app/javascript/users.js(1 hunks)app/views/activities/index.html.erb(1 hunks)app/views/activities/order_screen.html.erb(1 hunks)app/views/activities/show.html.erb(1 hunks)app/views/credit_mutations/index.html.erb(1 hunks)app/views/invoices/index.html.erb(1 hunks)app/views/layouts/application.html.erb(1 hunks)app/views/layouts/errors.html.erb(1 hunks)app/views/payments/add.html.erb(1 hunks)app/views/price_lists/index.html.erb(1 hunks)app/views/users/index.html.erb(1 hunks)app/views/users/show.html.erb(1 hunks)babel.config.js(0 hunks)bin/dev(1 hunks)bin/webpack(0 hunks)bin/webpack-dev-server(0 hunks)config/environments/development.rb(1 hunks)config/environments/production.rb(0 hunks)config/webpack/development.js(0 hunks)config/webpack/environment.js(0 hunks)config/webpack/loaders/eslint.js(0 hunks)config/webpack/luxproduction.js(0 hunks)config/webpack/production.js(0 hunks)config/webpack/staging.js(0 hunks)config/webpack/test.js(0 hunks)config/webpacker.yml(0 hunks)lib/generators/rails/webpacker_assets/USAGE(0 hunks)lib/generators/rails/webpacker_assets/webpacker_assets_generator.rb(0 hunks)package.json(2 hunks)webpack.config.js(1 hunks)
💤 Files with no reviewable changes (21)
- config/webpack/environment.js
- config/webpack/luxproduction.js
- app/javascript/packs/credit_mutations.js
- babel.config.js
- config/webpacker.yml
- config/webpack/development.js
- app/javascript/packs/users.js
- bin/webpack-dev-server
- config/webpack/loaders/eslint.js
- config/webpack/production.js
- app/javascript/packs/activity.js
- app/javascript/packs/payment_add.js
- config/environments/production.rb
- bin/webpack
- config/webpack/test.js
- app/javascript/packs/user.js
- app/javascript/packs/invoices.js
- lib/generators/rails/webpacker_assets/webpacker_assets_generator.rb
- lib/generators/rails/webpacker_assets/USAGE
- app/assets/javascripts/application.js
- config/webpack/staging.js
🧰 Additional context used
🧬 Code graph analysis (2)
app/javascript/activities.js (1)
app/javascript/price_lists.js (2)
element(4-4)priceLists(6-6)
app/javascript/credit_mutations.js (8)
app/javascript/activity.js (1)
element(6-6)app/javascript/invoices.js (1)
element(7-7)app/javascript/order_screen.js (1)
element(8-8)app/javascript/user.js (1)
element(7-7)app/javascript/users.js (1)
element(5-5)app/javascript/activities.js (1)
element(4-4)app/javascript/payment_add.js (1)
element(4-4)app/javascript/price_lists.js (1)
element(4-4)
🪛 LanguageTool
README.md
[grammar] ~8-~8: Ensure spelling is correct
Context: ...currently "Alpha AMBER") or through the builtin login portal to see how much credit the...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_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)
- GitHub Check: Build
🔇 Additional comments (35)
app/views/layouts/errors.html.erb (1)
6-7: LGTM: Turbo tracking + deferred script.
Matches the Rails 7/Turbo setup and keeps error pages consistent..gitignore (1)
40-41: All verification checks passed; code changes are correct.Builds are properly ignored while preserving the
.keepfile, the Sprockets manifest correctly links the builds directory, and the.keepfile is committed.Gemfile (1)
6-7: LGTM: Modern Rails 7 asset pipeline gems added correctly.The addition of
cssbundling-rails,dartsass-rails,jsbundling-rails,sprockets-rails, andturbo-railsaligns perfectly with the migration from Webpacker to the Rails 7+ asset pipeline and from Turbolinks to Turbo.Also applies to: 15-15, 40-41
app/javascript/user.js (1)
6-20: LGTM: Correct Turbo-based Vue initialization pattern.The element existence check and dataset-based initialization follow best practices for Turbo navigation. The structure ensures the Vue instance is only created when the required DOM element is present.
app/javascript/activities.js (1)
3-42: LGTM: Successful migration from Turbolinks to Turbo.The event listener has been correctly updated to
turbo:load, and the element existence guard ensures the Vue instance is only initialized when the modal is present. The data sourcing fromelement.dataset.priceListsfollows the new pattern established across the codebase.app/views/layouts/application.html.erb (2)
5-13: LGTM: Layout correctly updated for Turbo and Rails 7 asset pipeline.The changes appropriately modernize the asset loading strategy:
- Viewport meta tag for responsive design
data-turbo-trackreplacingdata-turbolinks-trackyield(:head)enabling per-page script injection
10-11: All per-page scripts already usedefer: trueconsistently.Verification confirms that every
javascript_include_tagcall across the application—including the main layout and all per-page scripts—already includes thedefer: trueattribute. The codebase is correctly configured for Turbo initialization and consistent script loading behavior.app/javascript/price_lists.js (1)
3-7: LGTM: Successful migration to Turbo with code quality improvements.The migration to
turbo:loadis correct, and the refactoring fromvartoconst/letmodernizes the code and improves maintainability. The use ofthis.priceListsensures proper Vue reactivity.Also applies to: 31-31, 41-41, 59-59, 68-68, 71-71, 128-128
webpack.config.js (1)
9-16: The auto-discovery pattern is well-structured and appropriate.Verification confirms the architecture intentionally separates entry points (root .js files) from shared code (subdirectories like
components/). All 10 discovered files are legitimate page/feature bundles, not utilities or helpers. No changes needed.package.json (6)
5-8: Build scripts look good for jsbundling-rails migration.The webpack and sass build scripts are correctly configured for the Rails 7 asset pipeline. The
build:cssscript appropriately outputs toapp/assets/builds/and loads fromnode_modules.
18-18: Good addition for Turbolinks migration.Adding
@hotwired/turbo-railsis correct for the migration from Turbolinks to Turbo in Rails 7.
46-46: Appropriate addition for Vue component styling.The
vue-style-loaderis correctly added as a devDependency for handling Vue component styles.
23-23: Good additions for the build toolchain.
babel-plugin-macrosenables Babel macro supportsassprovides Dart Sass for stylesheet compilation via thebuild:cssscriptAlso applies to: 33-33
17-17: No issues found—version is current and stable.The specified version 7.1.0 is the latest stable release of @fortawesome/fontawesome-free, and the package.json specification is correct.
39-40: Webpack 5 upgrade verified as compatible.All dependencies and configurations are compatible with webpack 5:
webpack.config.jsuses webpack 5 native APIs correctlyvue-loader@^15.11.1supports webpack 5 since v15.11.0 added explicit Webpack 5 compatibilitybabel-loader@^8.4.1and.babelrcconfiguration are webpack 5-compatiblewebpack-cli@^6.0.1is the correct version for webpack 5No critical compatibility issues found.
app/assets/stylesheets/order_screen.scss (1)
1-1: Correct Bootstrap import path for Dart Sass.The updated import path
@import 'bootstrap/scss/bootstrap'is correct for loading Bootstrap fromnode_moduleswith the Dart Sass compiler.bin/dev (2)
8-9: Good default PORT handling.The PORT default is correctly set using shell parameter expansion.
11-11: Verify the --env /dev/null flag is intentional.The
--env /dev/nullflag explicitly prevents foreman from loading a.envfile. If your project uses.envfiles for configuration, this may skip important environment variables.If you need environment variables from
.env, consider:-exec foreman start -f Procfile.dev --env /dev/null "$@" +exec foreman start -f Procfile.dev "$@"Or explicitly specify the .env file:
-exec foreman start -f Procfile.dev --env /dev/null "$@" +exec foreman start -f Procfile.dev --env .env "$@"app/views/activities/index.html.erb (1)
2-4: Correct pattern for Turbo-based JavaScript loading.The content_for :head block with
javascript_include_tagproperly loads the activities script with Turbo tracking and defer.app/views/activities/show.html.erb (1)
2-4: Correct pattern for Turbo-based JavaScript loading.The JavaScript include uses the singular "activity" asset name, which should match the corresponding entrypoint file.
Procfile.dev (2)
2-2: JS watcher correctly configured.The js process properly runs the webpack watcher with development environment.
3-3: CSS watcher correctly configured.The css process properly runs the sass watcher for stylesheet compilation.
app/views/payments/add.html.erb (1)
2-4: Correct pattern for Turbo-based JavaScript loading.The payment_add JavaScript is properly loaded in the head with Turbo tracking.
app/views/invoices/index.html.erb (1)
2-4: Correct pattern for Turbo-based JavaScript loading.The invoices JavaScript is properly loaded in the head with Turbo tracking.
app/assets/stylesheets/application.scss (1)
1-6: LGTM! Clean migration to npm-based asset imports.The Bootstrap and Font Awesome imports have been correctly updated to reference npm package paths, which aligns with the move away from Webpacker to the modern Rails asset pipeline.
app/views/users/show.html.erb (1)
2-4: LGTM! Proper asset loading for Turbo.The head block correctly loads the user-specific JavaScript with
data-turbo-track: "reload"anddefer: true, which ensures proper script execution timing and enables Turbo Drive to track asset changes.app/views/credit_mutations/index.html.erb (1)
2-4: LGTM! Consistent asset loading pattern.The head block follows the same pattern as other views in this PR, properly integrating with Turbo Drive's asset tracking and deferred script loading.
app/views/activities/order_screen.html.erb (1)
2-4: LGTM! Consistent with the migration pattern.The asset loading follows the established pattern across all migrated views.
app/views/price_lists/index.html.erb (1)
2-4: LGTM! Consistent asset loading.app/assets/config/manifest.js (1)
1-2: LGTM! Correct manifest configuration for the new asset pipeline.The change from
link_directorytolink_tree ../buildsproperly reflects the migration to jsbundling-rails and cssbundling-rails, where compiled assets are output to theapp/assets/buildsdirectory.app/views/users/index.html.erb (1)
2-4: LGTM! Correct Turbo asset loading pattern.The head content block correctly loads the JavaScript asset with Turbo tracking and deferred loading, following Rails 7 best practices for the migration from Webpacker.
app/javascript/order_screen.js (4)
1-6: LGTM! Import structure updated correctly.The migration to relative imports for components aligns with the new module structure in the Turbo-based setup.
7-9: LGTM! Correct Turbo event handler pattern.The migration from
turbolinks:loadtoturbo:loadwith proper element existence checking follows the correct pattern for the Turbo migration.
10-13: LGTM! Improved variable declarations.Changing from
vartoconstfor dataset-derived variables is a good practice that prevents accidental reassignment.
26-304: ****Vue-resource is already properly installed and configured. The
app/javascript/application.jsfile imports vue-resource and registers it withVue.use(VueResource)on line 12. CSRF token setup forVue.httpis also in place on lines 15-20.The code in
order_screen.jswill work correctly as this module loads afterapplication.jsexecutes its global setup. No changes are required.Likely an incorrect or invalid review comment.
| import VueResource from 'vue-resource'; | ||
| import axios from 'axios'; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the need for both VueResource and axios.
Both VueResource and axios are imported and configured with CSRF tokens. Having two HTTP clients can lead to maintenance confusion and increased bundle size. Please verify that both are necessary, or consider standardizing on one.
Run the following script to check usage patterns:
Also applies to: 12-12, 18-19
🏁 Script executed:
#!/bin/bash
# Check usage of VueResource vs axios in the codebase
echo "=== VueResource usage (this.\$http) ==="
rg -n --type=js --type=vue -C2 '\$http\.(get|post|put|delete|patch)'
echo -e "\n=== Axios usage ==="
rg -n --type=js --type=vue -C2 'axios\.(get|post|put|delete|patch)'Length of output: 115
I need to retry with a broader search strategy since the .vue file type isn't recognized in the sandbox. Let me search more comprehensively:
---
🏁 Script executed:
```shell
#!/bin/bash
# Broader search for VueResource and axios usage without file type restrictions
echo "=== Checking all references to \$http (VueResource pattern) ==="
rg -n '\$http\.(get|post|put|delete|patch)' --max-count=10
echo -e "\n=== Checking all references to axios method calls ==="
rg -n 'axios\.(get|post|put|delete|patch)' --max-count=10
echo -e "\n=== Checking for Vue.use patterns ==="
rg -n 'Vue\.use' --max-count=10
echo -e "\n=== Checking for any import/require of these libraries ==="
rg -n '(vue-resource|axios)' app/javascript/ --max-count=15
echo -e "\n=== List all files in app/javascript directory ==="
find app/javascript -type f -name '*.js' -o -name '*.vue' -o -name '*.ts' -o -name '*.tsx' | head -20
Length of output: 3766
Both VueResource and axios are actively used; consolidate to a single HTTP client.
Your code imports and configures two separate HTTP clients: VueResource (10 usages across price_lists.js, order_screen.js, and Vue components) and axios (4 usages in newer Vue components). This creates maintenance overhead and increases bundle size.
Consolidate to a single client—axios is the modern standard. Migrate remaining VueResource calls (this.$http) to axios.
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:
|
There was a problem hiding this comment.
Actionable comments posted: 11
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)
7-415: Critical: Potential memory leak with Turbo navigation.The
turbo:loadevent fires on every navigation, not just initial page load. When users navigate away and return to this page, new Vue instances will be created without destroying the previous ones, causing memory leaks and potential conflicts.Recommended solution:
Add cleanup on Turbo's cache/navigation events:
+let vueInstances = []; + document.addEventListener('turbo:load', () => { const element = document.getElementById('order-screen'); if (element != null) { + // Clean up existing instances before creating new ones + vueInstances.forEach(instance => { + if (instance && typeof instance.$destroy === 'function') { + instance.$destroy(); + } + }); + vueInstances = []; + const users = JSON.parse(element.dataset.users); // ... rest of setup ... const app = new Vue({ // ... app config ... }); + vueInstances.push(app); // For each additional Vue instance: - new Vue({ + const creditMutationModal = new Vue({ el: document.getElementById('credit-mutation-modal'), // ... }); + vueInstances.push(creditMutationModal); // Repeat for other instances... } }); + +// Clean up before Turbo caches the page +document.addEventListener('turbo:before-cache', () => { + vueInstances.forEach(instance => { + if (instance && typeof instance.$destroy === 'function') { + instance.$destroy(); + } + }); + vueInstances = []; +});
♻️ Duplicate comments (6)
app/javascript/application.js (1)
5-6: Dual HTTP client issue remains unresolved.Both VueResource and axios are still imported and configured with CSRF tokens. As noted in the previous review, having two HTTP clients increases bundle size and maintenance overhead. This issue should be addressed by consolidating to a single HTTP client (axios is recommended as the modern standard).
Also applies to: 12-12, 24-25
Procfile.dev (1)
1-3: LGTM; resolves prior port mismatch.No hardcoded -p; server will pick PORT from env/puma.rb. This addresses the earlier “-p 5000 vs bin/dev” concern.
Quick check that scripts exist:
rg -nP '"watch"\s*:|build:css' package.json -C1webpack.config.js (3)
18-21: Do not hardcode production mode.Use env/argv to switch modes for fast dev builds and correct prod optimizations.
-module.exports = { - mode: "production", - devtool: process.env.NODE_ENV === "production" ? "hidden-source-map" : "source-map", +module.exports = (_env, argv) => { + const mode = argv?.mode || process.env.NODE_ENV || "development"; + return { + mode, + devtool: mode === "production" ? "hidden-source-map" : "source-map", // ... -}; + }; +};
30-33: Resolve conflict: single chunk vs splitChunks.LimitChunkCount(maxChunks: 1) disables code splitting while splitChunks: 'all' tries to split. Choose one.
Option A (single bundle per entry):
plugins: [ - new webpack.optimize.LimitChunkCountPlugin({ maxChunks: 1 }), + new webpack.optimize.LimitChunkCountPlugin({ maxChunks: 1 }), new VueLoaderPlugin(), ], // ... optimization: { - splitChunks: { - chunks: 'all', - }, + // disabled (single file per entry) },Option B (enable shared chunks):
plugins: [ - new webpack.optimize.LimitChunkCountPlugin({ maxChunks: 1 }), new VueLoaderPlugin(), ], optimization: { splitChunks: { chunks: 'all', + cacheGroups: { vendor: { test: /[\\/]node_modules[\\/]/, name: 'vendors', priority: 10 } }, }, },Also applies to: 66-70
51-57: SVGs are routed to webfonts; likely incorrect.The font rule captures .svg and emits to webfonts/, which will also catch image SVGs.
Apply:
- { - test: /\.(woff|woff2|eot|ttf|otf|svg)$/i, + { + test: /\.(woff|woff2|eot|ttf|otf)$/i, type: 'asset/resource', generator: { filename: 'webfonts/[name][ext]' } }, + { + test: /\.svg$/i, + type: 'asset/resource', + generator: { filename: 'images/[name][ext]' } + },.babelrc (1)
13-17: transform‑runtime requires @babel/runtime at runtime.Add @babel/runtime to dependencies (not devDependencies). Also make intent explicit to avoid polyfill overlap with useBuiltIns.
#!/bin/bash jq -r '.dependencies["@babel/runtime"] // .devDependencies["@babel/runtime"] // "MISSING: @babel/runtime"' package.json- "@babel/plugin-transform-runtime" + ["@babel/plugin-transform-runtime", { "corejs": false, "helpers": true, "regenerator": true }]
🧹 Nitpick comments (7)
config/initializers/assets.rb (1)
6-6: Consider using single quotes for static strings.Ruby style conventions prefer single quotes for static strings without interpolation. While this is functionally equivalent, using single quotes here would be more consistent with typical Ruby style guides.
-Rails.application.config.assets.version = "1.0" +Rails.application.config.assets.version = '1.0'app/javascript/invoices.js (1)
46-70: Review the row cloning logic for robustness.The
addRowmethod manipulates the DOM directly to clone invoice rows and update field names. A few concerns:
- Line 64: The regex
/\[\d+\]/greplaces ALL bracketed digit sequences in the name attribute, which could inadvertently affect unrelated parts of the field name if the naming convention changes.- Line 48:
lastElementChildassumes there's always at least one row; this could fail if the container is empty.- The logic only updates
firstElementChildof each field wrapper, which might miss nested inputs.Consider adding defensive checks and making the regex more specific:
addRow() { const invoiceRowContainer = document.getElementById('invoice_row'); + if (!invoiceRowContainer || invoiceRowContainer.children.length === 0) { + console.error('Cannot add row: container is empty'); + return; + } const lastRow = invoiceRowContainer.lastElementChild; const newRow = lastRow.cloneNode(true); invoiceRowContainer.appendChild(newRow); Array.from(newRow.children).forEach((fieldWrapper) => { let newIndex = -1; if (fieldWrapper.firstElementChild) { const nameAttr = fieldWrapper.firstElementChild.name; if (nameAttr) { if (newIndex === -1) { - const match = nameAttr.match(/invoice\[rows_attributes\]\[(\d+)\]/); + const match = nameAttr.match(/invoice\[rows_attributes\]\[(\d+)\]\[/); if (match && match[1]) { newIndex = Number(match[1]) + 1; } } if (newIndex !== -1) { - fieldWrapper.firstElementChild.name = nameAttr.replace(/\[\d+\]/g, '[' + newIndex + ']'); + fieldWrapper.firstElementChild.name = nameAttr.replace( + /\[rows_attributes\]\[\d+\]/, + `[rows_attributes][${newIndex}]` + ); fieldWrapper.firstElementChild.value = ''; } } } }); }app/javascript/order_screen.js (1)
15-18: Minor: Globalwindow.flashfunction could be better scoped.The
window.flashfunction is set on everyturbo:loadevent. While reassignment is generally safe, consider using a namespaced approach (e.g.,window.app.flash) or checking if it already exists to avoid potential conflicts with other pages or libraries.Consider this pattern:
- window.flash = function(message, actionText, type) { + if (!window.flash) { + window.flash = function(message, actionText, type) { const event = new CustomEvent('flash', { detail: { message: message, actionText: actionText, type: type } } ); document.body.dispatchEvent(event); + }; - }; + }Or use a namespace:
- window.flash = function(message, actionText, type) { + window.orderScreen = window.orderScreen || {}; + window.orderScreen.flash = function(message, actionText, type) { const event = new CustomEvent('flash', { detail: { message: message, actionText: actionText, type: type } } ); document.body.dispatchEvent(event); };webpack.config.js (3)
10-16: Entries only scan the top level; nested entry points are missed.fs.readdirSync won’t find app/javascript/subdir/*.js. If you rely on nested entries, switch to a recursive glob.
Example:
-const entries = fs.readdirSync(javascriptsDir) - .filter(file => file.endsWith(".js")) - .reduce((acc, file) => { - const name = file.replace(/\.js$/, ""); - acc[name] = path.join(javascriptsDir, file); - return acc; - }, {}); +const glob = require("glob"); +const entries = glob.sync(path.join(javascriptsDir, "**/*.js")) + .reduce((acc, absPath) => { + const rel = path.relative(javascriptsDir, absPath).replace(/\.js$/, ""); + acc[rel] = absPath; + return acc; + }, {});If you prefer staying top‑level, confirm all intended entries live directly under app/javascript.
61-64: Prefer Vue runtime‑only build if SFCs are compiled.If templates are compiled by vue-loader, you don’t need the full compiler build; runtime is smaller.
- alias: { - vue$: 'vue/dist/vue.esm.js' - } + alias: { + vue$: 'vue/dist/vue.runtime.esm.js' + }Confirm no code relies on runtime template compilation (e.g., new Vue({ template: ... })).
61-62: Add .jsx to resolve extensions.You transpile .jsx but it won’t resolve without explicit extension.
- extensions: [".js", ".vue"], + extensions: [".js", ".jsx", ".vue"],.babelrc (1)
14-14: Optional: remove syntax-dynamic-import.Webpack handles dynamic import; Babel 7 parses it without extra plugin in most setups. Safe to drop if tests still pass.
- "@babel/plugin-syntax-dynamic-import", + // "@babel/plugin-syntax-dynamic-import", // likely unnecessary
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.babelrc(1 hunks).gitignore(1 hunks)Procfile.dev(1 hunks)README.md(1 hunks)app/assets/stylesheets/application.scss(1 hunks)app/javascript/activity.js(1 hunks)app/javascript/application.js(1 hunks)app/javascript/credit_mutations.js(1 hunks)app/javascript/invoices.js(1 hunks)app/javascript/order_screen.js(4 hunks)app/javascript/payment_add.js(1 hunks)app/javascript/users.js(1 hunks)app/views/activities/_credit_mutation_modal.html.erb(1 hunks)app/views/activities/index.html.erb(3 hunks)app/views/activities/order_screen.html.erb(8 hunks)app/views/activities/show.html.erb(3 hunks)app/views/credit_mutations/index.html.erb(3 hunks)app/views/index/index.html.erb(4 hunks)app/views/partials/_navigation_bar.html.erb(3 hunks)app/views/price_lists/index.html.erb(8 hunks)app/views/users/index.html.erb(4 hunks)app/views/users/show.html.erb(5 hunks)bin/dev(1 hunks)config/environments/development.rb(2 hunks)config/initializers/assets.rb(1 hunks)config/puma.rb(1 hunks)webpack.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- app/views/users/index.html.erb
- README.md
- app/javascript/users.js
- app/views/activities/index.html.erb
- app/javascript/credit_mutations.js
- app/javascript/payment_add.js
- app/views/credit_mutations/index.html.erb
- app/views/activities/order_screen.html.erb
- app/views/users/show.html.erb
- app/assets/stylesheets/application.scss
🧰 Additional context used
🧬 Code graph analysis (2)
app/javascript/activity.js (8)
app/javascript/credit_mutations.js (1)
element(16-16)app/javascript/invoices.js (1)
element(7-7)app/javascript/order_screen.js (2)
element(8-8)app(26-304)app/javascript/payment_add.js (1)
element(6-6)app/javascript/users.js (1)
element(14-14)app/javascript/activities.js (1)
element(4-4)app/javascript/user.js (1)
element(7-7)app/javascript/price_lists.js (1)
element(4-4)
config/initializers/assets.rb (1)
config/application.rb (1)
config(10-93)
🪛 ESLint
app/javascript/application.js
[error] 16-16: Expected indentation of 4 spaces but found 6.
(indent)
[error] 18-18: Expected indentation of 0 spaces but found 2.
(indent)
webpack.config.js
[error] 1-1: 'require' is not defined.
(no-undef)
[error] 2-2: 'require' is not defined.
(no-undef)
[error] 3-3: 'require' is not defined.
(no-undef)
[error] 4-4: 'require' is not defined.
(no-undef)
[error] 7-7: '__dirname' is not defined.
(no-undef)
[error] 18-18: 'module' is not defined.
(no-undef)
[error] 20-20: 'process' is not defined.
(no-undef)
[error] 27-27: '__dirname' is not defined.
(no-undef)
[error] 47-47: Expected indentation of 8 spaces but found 9.
(indent)
[error] 48-48: Expected indentation of 8 spaces but found 9.
(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 (21)
.gitignore (2)
35-36: Correct Webpacker→Rails 7 asset pipeline entries.The removals of
/public/packs*(Webpacker output) and addition of/app/assets/builds/*with a.keepexception correctly reflect the migration to Rails 7 JS bundling. This pattern is appropriate.Also applies to: 40-41
35-41: Review comment is incorrect:/node_modulesis NOT being removed from.gitignore.The snippet you provided as the "final state" actually demonstrates that
/node_modules(line 39),yarn-debug.log*(line 40), and.yarn-integrity(line 41) remain in.gitignore. Your concern about their removal contradicts the code snippet you quoted. The actual changes are correct: removing Webpacker entries (/public/packs*) and adding Rails 7+ jsbundling output (/app/assets/builds/*).Likely an incorrect or invalid review comment.
config/initializers/assets.rb (1)
1-14: Asset migration is correctly configured—no issues found.Verification confirms the Webpacker → webpack/Sass migration is complete and properly configured:
- Webpack correctly resolves npm packages (webpack searches
node_modulesby default for imports/requires)- Sass build explicitly includes
--load-path=node_modulesfor CSS imports from npm packages- All built assets output to
app/assets/builds/as configured in manifest.js- Asset pipeline serves pre-built assets through standard Rails asset tags (
stylesheet_link_tag,javascript_include_tag)- No old Webpacker pack tags remain
Removing the node_modules asset load path from
config/initializers/assets.rbis correct for this setup—the asset bundlers handle npm package resolution independently, and the Rails asset pipeline serves only the pre-built outputs.app/views/activities/show.html.erb (2)
2-4: LGTM! Asset loading updated for Turbo.The addition of the head content block to load the activity JavaScript with Turbo tracking and defer is consistent with the migration to Rails 7 + Turbo.
13-13: LGTM! Icon rendering modernized.The replacement of
fa_iconhelpers with inline FontAwesome<i>elements is consistent with the broader migration away from the font-awesome-rails gem.Also applies to: 17-17, 22-22, 81-81
app/views/activities/_credit_mutation_modal.html.erb (1)
19-19: LGTM! Icon rendering updated.Consistent with the project-wide migration from
fa_iconhelpers to inline FontAwesome elements.app/views/index/index.html.erb (1)
30-30: LGTM! Icon rendering modernized.The replacement of
fa_iconhelpers with inline FontAwesome<i>elements is consistent with the project-wide migration.Also applies to: 61-61, 89-89
app/views/price_lists/index.html.erb (3)
2-4: LGTM! Asset loading updated for Turbo.The head content block properly loads the price_lists JavaScript with Turbo tracking and defer.
31-31: Good catch on the typo fix!Correcting "gearchieveerde" to "gearchiveerde" improves the Dutch text quality.
22-22: LGTM! Icon rendering modernized.All icon changes from
fa_iconhelpers to inline FontAwesome<i>elements are consistent with the project-wide migration.Also applies to: 58-58, 61-61, 89-89, 103-103, 114-114, 121-121, 124-124
app/javascript/application.js (1)
20-27: LGTM! Proper CSRF token handling.The turbo:load event properly reads the CSRF token from the meta tag and conditionally sets it on both HTTP clients, preventing errors when the token is missing.
app/views/partials/_navigation_bar.html.erb (1)
13-13: LGTM! Icon rendering modernized across navigation.All navigation icons have been consistently migrated from
fa_iconhelpers to inline FontAwesome<i>elements, aligning with the project-wide migration.Also applies to: 19-19, 26-26, 31-31, 38-38, 45-45, 52-52, 59-59, 66-66, 75-75
app/javascript/order_screen.js (4)
3-5: LGTM: Import paths correctly updated for the migration.The relative import paths are appropriate for the Rails 7 JS bundling setup and follow standard ES module conventions.
214-226: Good fix: Typo corrected fromaffilateKeytoaffiliateKey.The variable name now correctly spells "affiliate" and properly constructs the SumUp payment URL.
318-324: LGTM: Well-structured validation helper.Extracting the validation logic into a dedicated
isFormInvalid()method improves readability and maintainability. The method correctly combines native HTML5 form validation with business logic checks.
326-334: LGTM: Clean validation flow.The refactored validation correctly uses the helper method and maintains the validation state flag for potential UI feedback.
bin/dev (1)
8-11: No issues found; code is correct as-is.Verification confirms PORT is properly set in
bin/dev(line 9) and correctly consumed bypuma.rb(line 27) viaENV.fetch('PORT', 5000). No conflicting port overrides detected elsewhere in the codebase. The optional suggestion to explicitly pass-p "$PORT"in Procfile.dev remains a valid belt-and-suspenders approach, but is not required.config/environments/development.rb (1)
22-22: Good fix: correct cache toggle path.Switching to tmp/caching-dev.txt is the standard Rails dev cache toggle.
.babelrc (3)
18-33: Test env block looks solid.Targets Node current and converts dynamic imports for tests; LGTM.
1-34: No action required—browserslist already configured.A
.browserslistrcfile exists with thedefaultsconfiguration, which properly controls polyfill targets. The recommendation is already satisfied.
6-8: core-js@3 is properly specified — concern addressed.Verification confirms core-js version 3 is present in package.json. The "useBuiltIns": "usage" configuration in .babelrc will correctly inject polyfills without runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/javascript/activity.js (1)
1-13: Previous memory leak issue resolved; remove unused import.The module-scoped
vueInstanceand top-levelturbo:before-cachelistener properly address the memory leak flagged in the previous review. This pattern now matches the approach used ininvoices.js,credit_mutations.js, andusers.js.However, the
Tabimport on line 4 is unused since the redundant tab activation code has been removed.Apply this diff to remove the unused import:
import Vue from 'vue/dist/vue.esm'; import ProductTotals from './components/activity/ProductTotals.vue'; -import Tab from 'bootstrap/js/dist/tab'; let vueInstance = null;config/initializers/assets.rb (1)
6-6: Optional: Prefer single quotes for static strings.Ruby convention favors single quotes when there's no string interpolation. Consider using
'1.0'instead of"1.0".app/javascript/invoices.js (1)
4-4: Consider migrating from moment.js to a modern alternative.Moment.js is in maintenance mode and has a large bundle size. Modern alternatives like
date-fnsorday.jsoffer better performance and tree-shaking.Example with
date-fns:-import moment from 'moment'; +import { format } from 'date-fns'; // ... formatDate(time) { - return moment(time).format('DD-MM-YY HH:mm'); + return format(new Date(time), 'dd-MM-yy HH:mm'); }Also applies to: 52-54
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/javascript/activity.js(1 hunks)app/javascript/application.js(1 hunks)app/javascript/invoices.js(1 hunks)bin/dev(1 hunks)config/initializers/assets.rb(1 hunks)webpack.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- webpack.config.js
- app/javascript/application.js
- bin/dev
🧰 Additional context used
🧬 Code graph analysis (3)
app/javascript/activity.js (3)
app/javascript/invoices.js (2)
vueInstance(6-6)element(16-16)app/javascript/credit_mutations.js (2)
vueInstance(5-5)element(16-16)app/javascript/users.js (2)
vueInstance(4-4)element(14-14)
app/javascript/invoices.js (4)
app/javascript/activity.js (2)
vueInstance(6-6)element(16-16)app/javascript/credit_mutations.js (2)
vueInstance(5-5)element(16-16)app/javascript/users.js (2)
vueInstance(4-4)element(14-14)app/javascript/activities.js (1)
element(4-4)
config/initializers/assets.rb (1)
config/application.rb (1)
config(10-93)
⏰ 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 (4)
app/javascript/activity.js (1)
15-25: LGTM! Redundant tab activation removed.The Vue initialization logic is clean and defensive. The redundant programmatic tab activation from the previous review has been properly removed, allowing the server-rendered active classes to handle the initial tab state.
config/initializers/assets.rb (2)
1-2: Past issue resolved.The work-in-progress marker "(CORRECT AND FINAL)" flagged in the previous review has been successfully removed. The file path comment is now clean and conventional.
10-10: LGTM! Correct Rails 7+ asset path configuration.Adding
app/assets/buildsto the asset load path is the standard approach for Rails 7+ with JS bundling (replacing Webpacker). This aligns correctly with the webpack configuration and manifest changes in the PR.app/javascript/invoices.js (1)
6-13: LGTM! Vue instance cleanup now properly implemented.The module-level
vueInstancevariable with cleanup onturbo:before-cacheaddresses the memory leak concern from the previous review. The pattern now matches the implementations inactivity.js,credit_mutations.js, andusers.js.
| addRow() { | ||
| const invoiceRowContainer = document.getElementById('invoice_row'); | ||
| const lastRow = invoiceRowContainer.lastElementChild; | ||
| const newRow = lastRow.cloneNode(true); | ||
|
|
||
| invoiceRowContainer.appendChild(newRow); | ||
| Array.from(newRow.children).forEach((fieldWrapper) => { | ||
| let newIndex = -1; | ||
| if (fieldWrapper.firstElementChild) { | ||
| const nameAttr = fieldWrapper.firstElementChild.name; | ||
| if (nameAttr) { | ||
| if (newIndex === -1) { | ||
| const match = nameAttr.match(/invoice\[rows_attributes\]\[(\d+)\]/); | ||
| if (match && match[1]) { | ||
| newIndex = Number(match[1]) + 1; | ||
| } | ||
| } | ||
| if (newIndex !== -1) { | ||
| fieldWrapper.firstElementChild.name = nameAttr.replace(/\[\d+\]/g, '[' + newIndex + ']'); | ||
| fieldWrapper.firstElementChild.value = ''; | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add null check to prevent runtime error.
Line 57's lastElementChild will be null if the container is empty, causing a TypeError on line 58 when cloneNode is called.
Apply this diff to add a defensive check:
addRow() {
const invoiceRowContainer = document.getElementById('invoice_row');
const lastRow = invoiceRowContainer.lastElementChild;
+ if (!lastRow) {
+ console.error('No invoice rows found to clone');
+ return;
+ }
const newRow = lastRow.cloneNode(true);📝 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.
| addRow() { | |
| const invoiceRowContainer = document.getElementById('invoice_row'); | |
| const lastRow = invoiceRowContainer.lastElementChild; | |
| const newRow = lastRow.cloneNode(true); | |
| invoiceRowContainer.appendChild(newRow); | |
| Array.from(newRow.children).forEach((fieldWrapper) => { | |
| let newIndex = -1; | |
| if (fieldWrapper.firstElementChild) { | |
| const nameAttr = fieldWrapper.firstElementChild.name; | |
| if (nameAttr) { | |
| if (newIndex === -1) { | |
| const match = nameAttr.match(/invoice\[rows_attributes\]\[(\d+)\]/); | |
| if (match && match[1]) { | |
| newIndex = Number(match[1]) + 1; | |
| } | |
| } | |
| if (newIndex !== -1) { | |
| fieldWrapper.firstElementChild.name = nameAttr.replace(/\[\d+\]/g, '[' + newIndex + ']'); | |
| fieldWrapper.firstElementChild.value = ''; | |
| } | |
| } | |
| } | |
| }); | |
| } | |
| addRow() { | |
| const invoiceRowContainer = document.getElementById('invoice_row'); | |
| const lastRow = invoiceRowContainer.lastElementChild; | |
| if (!lastRow) { | |
| console.error('No invoice rows found to clone'); | |
| return; | |
| } | |
| const newRow = lastRow.cloneNode(true); | |
| invoiceRowContainer.appendChild(newRow); | |
| Array.from(newRow.children).forEach((fieldWrapper) => { | |
| let newIndex = -1; | |
| if (fieldWrapper.firstElementChild) { | |
| const nameAttr = fieldWrapper.firstElementChild.name; | |
| if (nameAttr) { | |
| if (newIndex === -1) { | |
| const match = nameAttr.match(/invoice\[rows_attributes\]\[(\d+)\]/); | |
| if (match && match[1]) { | |
| newIndex = Number(match[1]) + 1; | |
| } | |
| } | |
| if (newIndex !== -1) { | |
| fieldWrapper.firstElementChild.name = nameAttr.replace(/\[\d+\]/g, '[' + newIndex + ']'); | |
| fieldWrapper.firstElementChild.value = ''; | |
| } | |
| } | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In app/javascript/invoices.js around lines 55-79, the code calls
lastElementChild.cloneNode but lastElementChild can be null when the container
is empty; add a defensive null check right after obtaining lastRow and bail out
or create/clone from a template when lastRow is null (e.g., if (!lastRow)
return; or use a predefined template element to clone), then proceed with the
existing renaming and clearing logic so cloneNode is only called on a non-null
element.
| } | ||
| } | ||
| if (newIndex !== -1) { | ||
| fieldWrapper.firstElementChild.name = nameAttr.replace(/\[\d+\]/g, '[' + newIndex + ']'); |
There was a problem hiding this comment.
Regex replacement may incorrectly modify nested array indices.
The global regex /\[\d+\]/g replaces all bracket-digit patterns in the field name. If the form contains nested arrays (e.g., invoice[rows_attributes][0][items][1][name]), this will incorrectly replace both the row index and nested indices.
Replace with a more specific pattern that targets only the rows_attributes index:
-fieldWrapper.firstElementChild.name = nameAttr.replace(/\[\d+\]/g, '[' + newIndex + ']');
+fieldWrapper.firstElementChild.name = nameAttr.replace(/(\[rows_attributes\]\[)\d+(\])/, '$1' + newIndex + '$2');📝 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.
| fieldWrapper.firstElementChild.name = nameAttr.replace(/\[\d+\]/g, '[' + newIndex + ']'); | |
| fieldWrapper.firstElementChild.name = nameAttr.replace(/(\[rows_attributes\]\[)\d+(\])/, '$1' + newIndex + '$2'); |
🤖 Prompt for AI Agents
In app/javascript/invoices.js around line 73, the current replacement uses a
global bracket-digit regex which will change every numeric index in nested
names; restrict the replacement to only the rows_attributes index by matching
the specific segment that contains rows_attributes and its following numeric
index (e.g. match the "[rows_attributes][<number>]" portion) and replace only
that captured numeric index with the newIndex while leaving other bracketed
numbers intact; implement the replacement using a regex that targets the
rows_attributes segment and a replacement that preserves the surrounding text so
nested array indices are not modified.
work in progress
Remove the old way of serving javascript through rails and replaced it with a rails 7 alternative
TO DO
Summary by CodeRabbit
New Features
Chores
Documentation
UI