[webpack] Web Apps #35594
[webpack] Web Apps #35594
Conversation
In requirejs, these dependencies were part of the requirejs config: https://github.com/dimagi/commcare-hq/blob/e999662c9c2d78d90e5dfdbf1e2d5b895edf4c40/corehq/apps/hqwebapp/static/hqwebapp/js/bootstrap5/requirejs_config.js#L37-L42
orangejenny
added
awaiting QA
orangejenny
requested review from
MartinRiese,
AddisonDunn,
stephherbers,
Robert-Costello,
Jtang-1 and
nospame
as code owners
dimagimon
added
the
Risk: Medium
| {% include "hqwebapp/partials/requirejs.html" %} | ||
| {% include "hqwebapp/partials/webpack.html" %} |
There was a problem hiding this comment.
Is it fine to be including both of these? Seems like it should be one or the other.
There was a problem hiding this comment.
It's not harmful, because all the logic in requirejs.html checks for a requirejs_main value, but yes it can be removed, I just missed it. ff214d8
|
|
||
| <style id="list-cell-layout-style"></style> | ||
| <!-- For Formplayer's app preview --> | ||
|
|
||
| {# This is fine as an inline script; it'll be removed once form designer is migrated to RequireJS #} | ||
| {# This is fine as an inline script; it'll be removed once all of HQ is using webpack #} |
|
I merged master to pull in this PR, then updated cloudcare tests to run using webpack insetad of requirejs. |
The repo-wide config uses 6, but that doesn't allow for "import" not at the top level, which is a parse error that then obscures all other lint errors.
WalkthroughThe pull request introduces a comprehensive set of changes to the CloudCare application's JavaScript codebase, focusing on modernizing the module loading approach, improving code syntax, and updating template rendering. The modifications primarily involve transitioning from RequireJS to Webpack, adding trailing commas for better code consistency, and simplifying module imports using dynamic Key changes include updating import statements, removing strict mode directives, modifying template rendering for badges, and adjusting JavaScript loading mechanisms in HTML templates. The changes span multiple files across the Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/views.js (1)
Line range hint
943-987: Ensure consistent usage of mapping librariesYou are setting
mapboxgl.accessToken = token;, but the map initialization and manipulation continue to use the Leaflet library (L). This could lead to inconsistencies or unexpected behavior due to mixing Mapbox GL JS and Leaflet. Consider using a single mapping library consistently or ensure that integrating both libraries is intentional and correctly implemented.
🧹 Nitpick comments (6)
corehq/apps/cloudcare/static/cloudcare/js/preview_app/dragscroll.js (2)
60-60: Refactor assignment out of expression.The assignment within the event listener expression makes the code less maintainable and could lead to confusion. Consider extracting the handler to a named function.
- _window[addEventListener]( - mouseup, cont.mu = function () {pushed = 0;}, 0, - ); + cont.mu = function () { + pushed = 0; + }; + _window[addEventListener](mouseup, cont.mu, 0);🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Line range hint
65-72: Improve readability of scroll position updates.The current implementation uses compound assignments that make the code harder to understand. Consider refactoring to improve clarity while maintaining the same functionality.
- if (pushed) { - (scroller = el.scroller || el).scrollLeft -= - (- lastClientX + (lastClientX = e.clientX)); - scroller.scrollTop -= - (- lastClientY + (lastClientY = e.clientY)); - } + if (pushed) { + const scroller = el.scroller || el; + const deltaX = e.clientX - lastClientX; + const deltaY = e.clientY - lastClientY; + + lastClientX = e.clientX; + lastClientY = e.clientY; + + scroller.scrollLeft -= -deltaX; + scroller.scrollTop -= -deltaY; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 70-70: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/api.js (2)
169-183: Consider improving error handling structure.While the trailing comma additions are good, there are opportunities to improve the error handling:
- Consider centralizing error messages in a constants file:
// errorConstants.js export const ERRORS = { FORM_SERVICE_CONNECTION: 'Unable to connect to form playing service...', // ... other error messages };
- Consider creating a unified error handling utility:
// errorUtils.js export const handleFormplayerError = (response, trigger) => { switch (response.status) { case 423: trigger('showError', errors.LOCK_TIMEOUT_ERROR); break; case 401: trigger('showError', formEntryUtils.reloginErrorHtml(), true); break; // ... other cases } };This would make the code more maintainable and consistent.
Line range hint
1-283: Consider modernizing the codebase during webpack transition.While transitioning to webpack, consider these architectural improvements:
- Replace jQuery promises with native Promises:
queryFormplayer: async function (params, route) { try { const appCollection = await AppsAPI.getAppEntities(); // ... rest of the logic } catch (error) { // ... error handling } }
- Use fetch API instead of jQuery.ajax:
const response = await fetch(formplayerUrl + '/' + route, { method: 'POST', headers: { 'Content-Type': 'application/json', }, body: JSON.stringify(data), });
- Consider breaking down the large
queryFormplayerfunction into smaller, more focused functions for better maintainability.These changes would make the code more modern and maintainable.
corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/main.js (1)
10-10: Consider adding error handling for mocha initialization.The direct call to
hqMocha.run()might benefit from error handling to gracefully handle test runner initialization failures.-hqMocha.run(); +try { + hqMocha.run(); +} catch (error) { + console.error('Failed to initialize test runner:', error); + throw error; +}corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/fixtures.js (1)
Line range hint
8-147: Consider adding input validation for fixture generators.The fixture generators accept options but lack input validation. Consider adding type checking and validation for critical parameters:
textJSON: (options = {}) => { + const requiredFields = ['binding', 'question_id']; + const missingFields = requiredFields.filter(field => + options[field] === undefined || options[field] === null + ); + if (missingFields.length > 0) { + throw new Error(`Missing required fields: ${missingFields.join(', ')}`); + } return _.defaults(options, {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (82)
corehq/apps/cloudcare/.eslintrc.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/config.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/debugger/debugger.js(5 hunks)corehq/apps/cloudcare/static/cloudcare/js/form_entry/const.js(0 hunks)corehq/apps/cloudcare/static/cloudcare/js/form_entry/entries.js(2 hunks)corehq/apps/cloudcare/static/cloudcare/js/form_entry/errors.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/form_entry/form_ui.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/entries_spec.js(4 hunks)corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/fixtures.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/form_ui_spec.js(2 hunks)corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/integration_spec.js(2 hunks)corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/main.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/utils_spec.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/web_form_session_spec.js(4 hunks)corehq/apps/cloudcare/static/cloudcare/js/form_entry/task_queue.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/form_entry/utils.js(2 hunks)corehq/apps/cloudcare/static/cloudcare/js/form_entry/web_form_session.js(5 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/app.js(6 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/apps/api.js(2 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/apps/collections.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/apps/controller.js(2 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/apps/models.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/apps/views.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/constants.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/hq_events.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/layout/views/progress_bar.js(2 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/layout/views/settings.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/main.js(2 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/api.js(4 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/collections.js(2 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/controller.js(3 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/utils.js(3 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/views.js(8 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/views/query.js(6 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/middleware.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/router.js(3 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/sessions/api.js(3 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/sessions/collections.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/sessions/controller.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/sessions/models.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/sessions/views.js(2 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/case_list_pagination_spec.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/debugger_spec.js(2 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/fake_formplayer.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/fixtures/case_grid_list.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/fixtures/case_list.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/fixtures/case_tile_list.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/fixtures/menu_list.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/fixtures/split_screen_case_list.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/hq_events_spec.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/main.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/menu_list_spec.js(6 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/menu_utils_spec.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/query_spec.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/session_middleware_spec.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/split_screen_case_search_spec.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/user_spec.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/utils_spec.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/users/collections.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/users/controller.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/users/models.js(3 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/users/utils.js(4 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/users/views.js(6 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/utils/calendar-picker-translations.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/formplayer/utils/utils.js(3 hunks)corehq/apps/cloudcare/static/cloudcare/js/markdown.js(4 hunks)corehq/apps/cloudcare/static/cloudcare/js/preview_app/dragscroll.js(3 hunks)corehq/apps/cloudcare/static/cloudcare/js/preview_app/main.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/preview_app/preview_app.js(2 hunks)corehq/apps/cloudcare/static/cloudcare/js/sentry.js(2 hunks)corehq/apps/cloudcare/static/cloudcare/js/spec/markdown_spec.js(6 hunks)corehq/apps/cloudcare/static/cloudcare/js/spec/utils_spec.js(1 hunks)corehq/apps/cloudcare/static/cloudcare/js/utils.js(6 hunks)corehq/apps/cloudcare/templates/cloudcare/formplayer_home.html(1 hunks)corehq/apps/cloudcare/templates/cloudcare/partials/all_templates.html(0 hunks)corehq/apps/cloudcare/templates/cloudcare/partials/menu/audio.html(1 hunks)corehq/apps/cloudcare/templates/cloudcare/partials/menu/badge.html(0 hunks)corehq/apps/cloudcare/templates/cloudcare/partials/menu/grid_item.html(2 hunks)corehq/apps/cloudcare/templates/cloudcare/partials/menu/row.html(2 hunks)corehq/apps/cloudcare/templates/cloudcare/preview_app_base.html(3 hunks)corehq/apps/cloudcare/templates/cloudcare/spec/form_entry/mocha.html(1 hunks)corehq/apps/cloudcare/templates/cloudcare/spec/mocha.html(1 hunks)
⛔ Files not processed due to max files limit (1)
- webpack/webpack.common.js
💤 Files with no reviewable changes (3)
- corehq/apps/cloudcare/static/cloudcare/js/form_entry/const.js
- corehq/apps/cloudcare/templates/cloudcare/partials/menu/badge.html
- corehq/apps/cloudcare/templates/cloudcare/partials/all_templates.html
✅ Files skipped from review due to trivial changes (57)
- corehq/apps/cloudcare/static/cloudcare/js/form_entry/errors.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/hq_events_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/query_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/fixtures/split_screen_case_list.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/menu_utils_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/fixtures/menu_list.js
- corehq/apps/cloudcare/static/cloudcare/js/form_entry/task_queue.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/users/controller.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/fixtures/case_grid_list.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/apps/api.js
- corehq/apps/cloudcare/static/cloudcare/js/form_entry/form_ui.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/fixtures/case_list.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/main.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/middleware.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/sessions/views.js
- corehq/apps/cloudcare/.eslintrc.js
- corehq/apps/cloudcare/static/cloudcare/js/spec/utils_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/integration_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/router.js
- corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/utils_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/spec/markdown_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/users/models.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/sessions/models.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/constants.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/apps/controller.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/sessions/controller.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/fixtures/case_tile_list.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/hq_events.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/split_screen_case_search_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/config.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/layout/views/progress_bar.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/debugger_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/apps/views.js
- corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/form_ui_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/views/query.js
- corehq/apps/cloudcare/static/cloudcare/js/preview_app/preview_app.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/users/collections.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/users/utils.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/fake_formplayer.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/layout/views/settings.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/apps/models.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/controller.js
- corehq/apps/cloudcare/static/cloudcare/js/debugger/debugger.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/user_spec.js
- corehq/apps/cloudcare/templates/cloudcare/spec/form_entry/mocha.html
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/menu_list_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/session_middleware_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/collections.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/case_list_pagination_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/sessions/api.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/sessions/collections.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/apps/collections.js
- corehq/apps/cloudcare/static/cloudcare/js/markdown.js
- corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/web_form_session_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/entries_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/spec/utils_spec.js
- corehq/apps/cloudcare/static/cloudcare/js/form_entry/entries.js
👮 Files not reviewed due to content moderation or server errors (4)
- corehq/apps/cloudcare/static/cloudcare/js/utils.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/utils/utils.js
- corehq/apps/cloudcare/static/cloudcare/js/form_entry/web_form_session.js
- corehq/apps/cloudcare/static/cloudcare/js/formplayer/app.js
🧰 Additional context used
🪛 Biome (1.9.4)
corehq/apps/cloudcare/static/cloudcare/js/preview_app/dragscroll.js
[error] 60-60: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 65-72: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (python-sharded-and-javascript)
- GitHub Check: tests (python, bf)
- GitHub Check: tests (python, 6a)
- GitHub Check: tests (python, 05)
🔇 Additional comments (24)
corehq/apps/cloudcare/static/cloudcare/js/preview_app/dragscroll.js (1)
Line range hint
40-56: LGTM! Well-implemented form control handling.The monkeypatch effectively prevents drag behavior on form controls while maintaining the drag-scroll functionality for other elements. The implementation is robust with proper cross-browser compatibility.
corehq/apps/cloudcare/templates/cloudcare/partials/menu/audio.html (1)
Line range hint
5-24: LGTM: Consistent badge implementation across templates.The badge implementation is consistent across all menu templates, using proper XSS prevention with <%- %> for variable interpolation and maintaining consistent Bootstrap styling. The audio controls are well-structured with appropriate toggle functionality.
corehq/apps/cloudcare/static/cloudcare/js/formplayer/utils/calendar-picker-translations.js (2)
Line range hint
1-324: Verify webpack bundling of translationsSince we're transitioning to webpack, let's ensure all regional translations are properly included in the final bundle.
Run this script to verify translation bundling:
✅ Verification successful
Regional translations are properly bundled
All calendar translations (English, Amharic, Tigrinya, and Oromo) are correctly defined and accessible in the webpack bundle.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if webpack is properly handling all regional options rg --type js "regionalOptions\[('|\").*?('|\")\]" | sort -u # Verify that no translations are accidentally excluded during bundling rg --type js "\.calendars\.ethiopian\.prototype\.regionalOptions"Length of output: 2564
3-4: Verify jQuery plugins loading orderThe order of jQuery plugins is correct (jquery.plugin before jquery.calendars), but let's verify that webpack maintains this order during bundling.
Run this script to check the webpack configuration and ensure proper module loading order:
✅ Verification successful
jQuery plugins loading order is correctly maintained
The RequireJS configuration in both Bootstrap 3 and 5 environments ensures proper dependency loading order, with jquery.plugin consistently loaded before jquery.calendars and other calendar-related plugins.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check webpack configuration for module loading order rg -l "jquery\.calendars" | xargs rg "jquery\.plugin" # Check if there are any other files using these plugins rg --type js "jquery\.(plugin|calendars)"Length of output: 5815
corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/api.js (2)
78-78: LGTM! Improved error message readability.The added space after the period improves the readability of the error message.
74-75: 🛠️ Refactor suggestionConsider transitioning from RequireJS to ES modules.
Since this PR aims to transition from
hqRequireto webpack, consider updating this file to use ES modules:-hqDefine("cloudcare/js/formplayer/menus/api", [ +import $ from 'jquery'; +import _ from 'underscore'; +import * as Sentry from 'sentry_browser'; // ... other imports -], function ($, _, Sentry, ...) { + +export const API = { // ... rest of the code -});Likely invalid or redundant comment.
corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/main.js (1)
1-7: Verify import paths are correctly mapped in webpack configuration.The transition from RequireJS to webpack requires proper path resolution configuration. Ensure these import paths are correctly mapped in the webpack config:
- "mocha/js/main"
- "commcarehq"
- "cloudcare/js/form_entry/spec/*"
✅ Verification successful
Import paths are correctly structured and all modules exist
The import paths are valid as they correctly map to existing files in the codebase:
mocha/js/main→corehq/apps/mocha/static/mocha/js/main.jscommcarehq→corehq/apps/hqwebapp/static/hqwebapp/js/{bootstrap3|5}/commcarehq.js- All spec files exist in
corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check webpack configuration for path aliases and verify file existence fd -e js -e json webpack.config rg -l "resolve(.|\n)*alias" $(fd -e js -e json webpack.config)Length of output: 290
Script:
#!/bin/bash # Find webpack config files echo "=== Webpack configs ===" fd webpack.config # Check content of webpack configs for aliases echo -e "\n=== Alias configurations ===" rg -U "resolve.*\{.*alias.*\}" --multiline # Verify existence of imported modules echo -e "\n=== Imported modules ===" fd -t f "main.js$" -p "mocha/js/" fd -t f "commcarehq" fd -t f "entries_spec.js|form_ui_spec.js|integration_spec.js|utils_spec.js" -p "cloudcare/js/form_entry/spec/"Length of output: 66252
Script:
#!/bin/bash # Search for webpack configs and module files echo "=== Webpack configs ===" fd -e js -e json webpack.config echo -e "\n=== Module files ===" # Look for mocha main.js echo "Searching for mocha/main.js:" fd -t f "main.js$" # Look for commcarehq echo -e "\nSearching for commcarehq:" fd -t f "commcarehq.js$" # Look for spec files echo -e "\nSearching for spec files:" fd -t f "entries_spec.js|form_ui_spec.js|integration_spec.js|utils_spec.js"Length of output: 4144
corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/views.js (5)
18-19: Addition of 'mapbox-gl' and 'leaflet' imports is appropriateThe new dependencies
'mapbox-gl/dist/mapbox-gl'and'leaflet'are correctly added to the module imports and function parameters, which will enhance the mapping functionalities within the application.Also applies to: 38-39
137-137: Properly passing 'badgeText' and 'menuIndex' to template contextIncluding
badgeTextandmenuIndexin thetemplateContextensures that these values are accessible within the template, facilitating dynamic rendering based on the model's attributes.
1067-1067: Appropriate usage of collection length in paginationIncluding
this.options.collection.lengthlikely aids in pagination controls or displaying the number of items, which is appropriate.
1668-1671: Proper handling of 'transitionend' eventThe condition
if (this.menuExpanded && event.target === targetElement)correctly ensures that the class is removed only when the transition ends on the specific target element, preventing potential side effects from other transitions.
879-881: Verify the dynamic import and usage of 'alert_user' modulePlease confirm that the dynamic import of
'hqwebapp/js/bootstrap5/alert_user'and the usage ofalertUser.alert_userare correct. Ensure that the module exports analert_userfunction and that it is being accessed properly.Run the following script to verify the export of
alert_userfunction:✅ Verification successful
Dynamic import and usage of 'alert_user' module is correctly implemented
The module exports the expected
alert_userfunction with the correct signature, and the usagealertUser.alert_user(errorMessage, 'danger')is proper. The implementation includes robust features like message styling, fade-out, and proper DOM management.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find 'alert_user.js' module and check if it exports 'alert_user' function # Find 'alert_user.js' file fd 'alert_user.js' | while read file; do # Check if it exports 'alert_user' function ast-grep --lang javascript --pattern $'export function alert_user($_, $_) { $$$ }' "$file" doneLength of output: 543
Script:
#!/bin/bash # Check content and exports in bootstrap5 alert_user.js echo "=== File content ===" cat corehq/apps/hqwebapp/static/hqwebapp/js/bootstrap5/alert_user.js echo -e "\n=== Checking exports ===" ast-grep --pattern 'export' corehq/apps/hqwebapp/static/hqwebapp/js/bootstrap5/alert_user.jsLength of output: 3562
corehq/apps/cloudcare/static/cloudcare/js/preview_app/main.js (1)
8-8: Confirm the necessity and usage of 'commcarehq' moduleThe module
'commcarehq'has been added to the dependencies but is not referenced in the function parameters or used within the code. Please verify whether this module is required. If it provides side effects needed by the application, consider adding a comment to clarify its purpose.Also applies to: 14-14
corehq/apps/cloudcare/static/cloudcare/js/formplayer/main.js (2)
6-6: LGTM: Addition of commcarehq dependencyThe addition of the 'commcarehq' dependency aligns with the PR objective of transitioning to webpack.
28-28: LGTM: Code style improvementThe addition of a trailing comma improves git diff readability and follows modern JavaScript conventions.
corehq/apps/cloudcare/static/cloudcare/js/formplayer/users/views.js (2)
37-39: LGTM: Improved template ID assignment readabilityThe conditional expression for templateId has been split into multiple lines for better readability.
99-102: LGTM: Modernized module loadingReplaced
hqRequirewith modern dynamicimport()syntax, aligning with the webpack transition objective.corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/utils.js (2)
Line range hint
37-41: LGTM: Modernized module loadingSuccessfully transitioned from
hqRequireto dynamicimport()syntax for MenusController, maintaining the same functionality while aligning with webpack standards.
49-49: LGTM: Code style improvementAdded trailing comma in error handling parameters for better git diff readability.
corehq/apps/cloudcare/static/cloudcare/js/form_entry/utils.js (1)
59-62: LGTM: Enhanced module loading with Promise-based importsSuccessfully replaced multiple
hqRequirecalls with parallel dynamic imports using$.when. This change:
- Maintains parallel module loading capability
- Aligns with modern JavaScript practices
- Supports the webpack transition objective
corehq/apps/cloudcare/templates/cloudcare/spec/mocha.html (1)
3-3: LGTM: Clean transition to webpack.The change from
requirejs_main_b5tojs_entryis consistent with the webpack migration objective.corehq/apps/cloudcare/templates/cloudcare/preview_app_base.html (2)
5-5: LGTM: Consistent webpack migration changes.The changes correctly implement the transition to webpack:
- Replaced RequireJS with webpack entry point
- Added webpack dependencies
- Updated feature flag from USE_REQUIREJS to USE_WEBPACK
Also applies to: 53-53, 58-61
80-80: Verify notifications service integration.The addition of notifications service URL registration might affect the application's notification handling.
Run this script to check for notification service dependencies and usage:
✅ Verification successful
Notifications service integration is properly implemented ✓
The URL registration is consistent with the existing notifications service implementation used across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for notifications service usage in JavaScript files rg -l "notifications_service" --type js # Check for any existing notification handling code ast-grep --pattern 'notifications_service'Length of output: 815
corehq/apps/cloudcare/templates/cloudcare/formplayer_home.html (1)
43-43: LGTM: Consistent webpack integration.The change from
requirejs_main_b5tojs_entrymaintains consistency with other templates in the webpack migration.
| @@ -2,7 +2,7 @@ | |||
| <td class="module-column module-column-icon"> | |||
| <% if (imageUrl) { %> | |||
| <div class="module-icon-image badge-container" style="background-image: url('<%- imageUrl %>');"> | |||
| <%= _.template($('#maybe-custom-badge-template').text())(arguments[0]) %>{# maybe-custom-badge-template output is already escaped #} | |||
| <span id="module-badge" class="badge bg-dark rounded-pill"><%- badgeText %></span> | |||
There was a problem hiding this comment.
Replace duplicate IDs with classes for badge elements.
Using the same ID 'module-badge' multiple times in the template violates HTML specifications, as IDs must be unique within a document. Since these badges appear to share the same styling, convert the ID to a class.
Apply this change to both badge elements:
- <span id="module-badge" class="badge bg-dark rounded-pill"><%- badgeText %></span>
+ <span class="module-badge badge bg-dark rounded-pill"><%- badgeText %></span>Also applies to: 14-14
| @@ -2,7 +2,7 @@ | |||
| <div> | |||
| <% if (imageUrl) { %> | |||
| <div class="gridicon badge-container" style="background-image: url('<%- imageUrl %>');"> | |||
| <%= _.template($('#maybe-custom-badge-template').text())(arguments[0]) %>{# maybe-custom-badge-template output is already escaped #} | |||
| <span id="module-badge" class="badge bg-dark rounded-pill"><%- badgeText %></span> | |||
There was a problem hiding this comment.
Replace duplicate IDs with classes for badge elements.
The same ID 'module-badge' is used multiple times, which violates HTML specifications. Convert to a class to maintain proper HTML semantics while preserving the shared styling.
Apply this change to both badge elements:
- <span id="module-badge" class="badge bg-dark rounded-pill"><%- badgeText %></span>
+ <span class="module-badge badge bg-dark rounded-pill"><%- badgeText %></span>Also applies to: 14-14
| @@ -2,12 +2,12 @@ | |||
| <td class="module-column module-column-icon"> | |||
| <% if (imageUrl) { %> | |||
| <div class="module-icon-image badge-container" style="background-image: url('<%- imageUrl %>');"> | |||
| <%= _.template($('#maybe-custom-badge-template').text())(arguments[0]) %>{# maybe-custom-badge-template output is already escaped #} | |||
| <span id="module-badge" class="badge bg-dark rounded-pill"><%- badgeText %></span> | |||
There was a problem hiding this comment.
Replace duplicate IDs with classes for badge elements.
The same ID 'module-badge' is used multiple times, which violates HTML specifications. This issue is consistent across all menu templates (row.html, grid_item.html, and audio.html).
Apply this change to both badge elements:
- <span id="module-badge" class="badge bg-dark rounded-pill"><%- badgeText %></span>
+ <span class="module-badge badge bg-dark rounded-pill"><%- badgeText %></span>Also applies to: 10-10
| hqDefine('cloudcare/js/formplayer/utils/calendar-picker-translations', [ | ||
| 'jquery', | ||
| 'calendars/dist/js/jquery.plugin', | ||
| 'calendars/dist/js/jquery.calendars', |
There was a problem hiding this comment.
Transition from hqDefine to ES6 imports needed
According to the PR objectives, we should be transitioning from hqRequire to webpack imports. Please update the module definition to use ES6 imports.
Here's how to refactor this:
-hqDefine('cloudcare/js/formplayer/utils/calendar-picker-translations', [
- 'jquery',
- 'calendars/dist/js/jquery.plugin',
- 'calendars/dist/js/jquery.calendars',
+import $ from 'jquery';
+import 'calendars/dist/js/jquery.plugin';
+import 'calendars/dist/js/jquery.calendars';Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| hqDefine("cloudcare/js/form_entry/spec/fixtures", [ | ||
| "underscore", | ||
| ], function ( | ||
| _ | ||
| _, | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Migrate to ES6 module syntax for webpack compatibility.
This file still uses the legacy hqDefine pattern while the PR's objective is to transition to webpack. Consider migrating to ES6 modules:
-hqDefine("cloudcare/js/form_entry/spec/fixtures", [
- "underscore",
-], function (
- _,
-) {
- return {
+import _ from 'underscore';
+
+export default {Committable suggestion skipped: line range outside the PR's diff.
| Sentry, | ||
| SentryCaptureConsole, |
There was a problem hiding this comment.
Correct instantiation of 'CaptureConsole' integration
The usage of new SentryCaptureConsole.Integrations.CaptureConsole may be incorrect. Since SentryCaptureConsole likely exports the CaptureConsole integration directly, you should instantiate it without accessing .Integrations.CaptureConsole.
Apply this diff to fix the instantiation:
integrations: [
new Sentry.Integrations.Breadcrumbs({
dom: false,
console: false,
}),
- new SentryCaptureConsole.Integrations.CaptureConsole({
+ new SentryCaptureConsole({
levels: ["error"],
}),
],Also applies to: 29-31
orangejenny
added
QA Passed
and removed
Open for review: do not merge
Technical Description
This is not a whole lot of code, since web apps was already on requirejs. Mostly it's switching from the old
hqRequireapproach for circular dependencies to usingimportcalls, plus a few config updates relating to external libraries that haven't been used in a webpack context yet.https://dimagi.atlassian.net/browse/SAAS-16004
Safety Assurance
Safety story
This will go through a full regression test.
Automated test coverage
A little bit.
QA Plan
https://dimagi.atlassian.net/browse/QA-7384
Rollback instructions
Labels & Review