Refactor some components #1417
Refactor some components #1417
Conversation
…nInRowAction` in `object-list-view-row` component
|
Kudos, SonarCloud Quality Gate passed!
|
TeterinaSvetlana
changed the base branch from
feature-ember-update
to
develop
| @@ -98,14 +98,14 @@ export default FlexberryBaseComponent.extend({ | |||
| saveAdvLimit: function() { | |||
| this._hideMessage(); | |||
| const advLimitName = this.get('model.advLimitName'); | |||
| if (isBlank(advLimitName)) { | |||
| this._showMessage('warning', this.get('i18n').t('components.advlimit-dialog-content.enter-setting-name')); | |||
| if (Ember.isBlank(advLimitName)) { | |||
| return typeOf(caption) === 'string' && $.trim(caption) !== '' || | ||
| typeOf(isHTMLSafe) === 'function' && isHTMLSafe(caption) && $.trim(get(caption, 'string')) !== '' || | ||
| caption instanceof htmlSafe && $.trim(get(caption, 'string')) !== ''; | ||
| _hasCaption: Ember.computed('caption', function () { |
| _hasCaption: Ember.computed('caption', function () { | ||
| const caption = this.get('caption'); | ||
|
|
||
| if (Ember.isEmpty(caption)) return false; |
There was a problem hiding this comment.
isEmpty и добавить в импорт
|
|
||
| return isCaptionTypeIsString && isCaptionNotEmpty | ||
| || isHTMLSafeDefined && isCaptionSafe && isStringOfCaptionEmpty === false | ||
| || caption instanceof Ember.Handlebars.SafeString && isStringOfCaptionEmpty === false; |
There was a problem hiding this comment.
везде где Ember.что-то убрать Ember и добавить в импорт если надо
| @@ -1189,8 +1190,8 @@ export default FlexberryBaseComponent.extend({ | |||
| } | |||
| }; | |||
|
|
|||
| later((function() { | |||
| modelController.transitionToRoute(editFormRoute + '.new', transitionOptions); | |||
| Ember.run.later((function() { | |||
| @@ -255,8 +258,7 @@ export default FlexberryBaseComponent.extend({ | |||
| @property colsSettingsItems | |||
| @readOnly | |||
| */ | |||
| colsSettingsItems: computed('i18n.locale', 'userSettingsService.isUserSettingsServiceEnabled', function() { | |||
| let i18n = this.get('i18n'); | |||
| colsSettingsItems: Ember.computed('i18n.locale', 'userSettingsService.isUserSettingsServiceEnabled', function() { | |||
There was a problem hiding this comment.
везде где Ember.что-то убрать Ember и добавить в импорт если надо
| @@ -2,10 +2,9 @@ | |||
| @module ember-flexberry | |||
| */ | |||
|
|
|||
| import Mixin from '@ember/object/mixin'; | |||
There was a problem hiding this comment.
без этого импорта работает разве?
| @@ -71,13 +70,12 @@ export default Mixin.create({ | |||
| @property formSuccessMessageCaption. | |||
| @type String | |||
| */ | |||
| formSuccessMessageCaption: computed('i18n.locale', 'latestOperationType', function() { | |||
| let i18n = this.get('i18n'); | |||
| formSuccessMessageCaption: Ember.computed('i18n.locale', 'latestOperationType', function() { | |||
There was a problem hiding this comment.
везде где Ember.что-то убрать Ember и добавить в импорт если надо
| @@ -2,6 +2,8 @@ | |||
| @module ember-flexberry | |||
| */ | |||
|
|
|||
| import Ember from 'ember'; | |||
There was a problem hiding this comment.
этот импорт вроде не нужен
| @@ -4,6 +4,8 @@ | |||
|
|
|||
| import Mixin from '@ember/object/mixin'; | |||
| import { merge } from '@ember/polyfills'; | |||
| import Ember from 'ember'; | |||
WalkthroughThe changes in this pull request focus on enhancing localization and improving code clarity across multiple components and mixins in the Ember application. Key modifications include the introduction of the Changes
Poem
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: 17
🧹 Outside diff range and nitpick comments (10)
addon/components/mobile/flexberry-file.js (2)
Line range hint
4-5: Add missing import for translation macro.The code uses the
t()function but doesn't import it. This would cause runtime errors.Add the following import:
import { computed } from '@ember/object'; +import { t } from 'ember-i18n/macro'; import FlexberryFile from './../flexberry-file';
Line range hint
28-28: Consider removing unnecessary dependency.The computed property still depends on 'i18n.locale' but this might not be needed when using the
t()macro as it handles locale changes automatically.- _menuItems: computed('showPreview', 'readonly', 'i18n.locale', '_uploadButtonIsVisible', '_downloadButtonIsVisible', function() { + _menuItems: computed('showPreview', 'readonly', '_uploadButtonIsVisible', '_downloadButtonIsVisible', function() {addon/components/flexberry-button.js (1)
73-86: Consider extracting caption handling logic to a utility function.The caption validation logic could be beneficial for other components. Consider extracting it to a shared utility function to improve reusability and testability.
Example structure:
// utils/string-helpers.js export function hasValidCaption(caption) { if (isEmpty(caption)) { return false; } const trimmedCaption = $.trim(caption); if (typeOf(caption) === 'string') { return trimmedCaption !== ''; } if (isHTMLSafe(caption)) { return $.trim(get(caption, 'string')) !== ''; } return false; } // In component: _hasCaption: computed('caption', function() { return hasValidCaption(get(this, 'caption')); }),addon/components/mobile/object-list-view.js (1)
125-126: LGTM! Correct usage of translation macro.The replacement of
i18n.twith the translation macro is implemented correctly and maintains the existing functionality.Consider reducing duplication by combining the title and titleKey logic:
- let checkAllAtPageTitle = isUncheckAllAtPage ? t('components.olv-toolbar.uncheck-all-at-page-button-text') : - t('components.olv-toolbar.check-all-at-page-button-text'); - let checkAllAtPageTitleKey = isUncheckAllAtPage ? 'components.olv-toolbar.uncheck-all-at-page-button-text' : - 'components.olv-toolbar.check-all-at-page-button-text'; + let checkAllAtPageKey = `components.olv-toolbar.${isUncheckAllAtPage ? 'uncheck' : 'check'}-all-at-page-button-text`; + let checkAllAtPageTitle = t(checkAllAtPageKey); + let checkAllAtPageTitleKey = checkAllAtPageKey; - let checkAllTitle = allSelect ? t('components.olv-toolbar.uncheck-all-button-text') : - t('components.olv-toolbar.check-all-button-text'); - let checkAllTitleKey = allSelect ? 'components.olv-toolbar.uncheck-all-button-text' : - 'components.olv-toolbar.check-all-button-text'; + let checkAllKey = `components.olv-toolbar.${allSelect ? 'uncheck' : 'check'}-all-button-text`; + let checkAllTitle = t(checkAllKey); + let checkAllTitleKey = checkAllKey;Also applies to: 130-131
addon/mixins/edit-form-controller-operations-indication.js (1)
Line range hint
126-134: Potential XSS vulnerability in error message handlingThe error messages are directly inserted as HTML without sanitization. If
errorMessagescontains user input or external data, this could lead to XSS vulnerabilities.Consider either:
- Sanitizing the error messages before insertion
- Using plain text instead of HTML
- errorMessages.forEach((currentErrorMessage) => { - message += '<li>' + currentErrorMessage + '</li>'; + errorMessages.forEach((currentErrorMessage) => { + message += '<li>' + Ember.Handlebars.Utils.escapeExpression(currentErrorMessage) + '</li>'; });addon/components/mobile/flexberry-objectlistview.js (1)
280-284: Add null check for columnHeader.The code assumes
columnHeaderwill be defined after the translation call. Consider adding proper null checks to handle cases where the translation might not exist.- let columnHeader = t(getAttrLocaleKey(this.get('modelName'), this.get('modelProjection').projectionName, column.key)).string; - if (columnHeader !== undefined) { - let key = column.key.split('.')[0]; - columnHeader = t(getAttrLocaleKey(this.get('modelName'), this.get('modelProjection').projectionName, key)).string; - } + let columnHeader = t(getAttrLocaleKey(this.get('modelName'), this.get('modelProjection').projectionName, column.key))?.string; + if (!columnHeader) { + const key = column.key.split('.')[0]; + columnHeader = t(getAttrLocaleKey(this.get('modelName'), this.get('modelProjection').projectionName, key))?.string || key; + }addon/components/flexberry-base-component.js (1)
253-257: Add JSDoc documentation for the new method.The method should follow the existing documentation pattern in the file. Also, remove the extra blank line for consistency.
+ /** + Checks if the given component name matches the current component's name. + + @method isNameOfCurrentComponent + @param {String} componentName The name to compare against + @return {Boolean} True if names match, false otherwise + */ isNameOfCurrentComponent(componentName) { const currentComponentName = this.get('componentName'); - return currentComponentName === componentName; },addon/components/groupedit-toolbar.js (1)
Line range hint
272-280: Refactor DOM manipulation to follow Ember best practices.While the component name check improvement is good, there are some concerns with the DOM manipulation code:
- Using jQuery (
this.$()) for DOM manipulation is deprecated in modern Ember and should be avoided.- Direct DOM queries create tight coupling between the template and JavaScript logic.
Consider refactoring to use Ember's element modifiers or computed properties:
// In the component @tracked selectedIndex; @tracked totalRows; @computed('selectedIndex') get isFirstRowSelected() { return this.selectedIndex === 0; } @computed('selectedIndex', 'totalRows') get isLastRowSelected() { return this.selectedIndex === this.totalRows - 1; } _rowSelected(componentName, record, count, checked, recordWithKey) { if (this.isNameOfCurrentComponent(componentName) && !this.get('isDestroying')) { this.set('_hasSelectedRows', count > 0); // Update these properties from your row selection logic this.selectedIndex = /* index of selected row */; this.totalRows = /* total number of rows */; this.set('_disableMoveUpButton', this.isFirstRowSelected); this.set('_disableMoveDownButton', this.isLastRowSelected); } }This approach:
- Removes deprecated jQuery usage
- Decouples the logic from DOM structure
- Makes the code more maintainable and testable
addon/components/object-list-view-row.js (1)
282-292: LGTM! Consider enhancing error handling.The switch statement refactoring improves readability. However, consider enhancing the error handling:
customButtonInRowAction(action, model) { + if (!action) { + throw new Error('Action parameter is required for custom buttons in row.'); + } + switch (typeof action) { case 'function': + if (typeof model === 'undefined') { + throw new Error('Model parameter is required when executing function actions.'); + } action(model); break; case 'string': this.sendAction('customButtonInRowAction', action, model); break; default: - throw new Error('Unsupported action type for custom buttons in row.'); + throw new Error(`Unsupported action type '${typeof action}' for custom buttons in row. Expected 'function' or 'string'.`); } }addon/components/object-list-view.js (1)
1440-1440: Add missing semicolon after 'contentLength' declarationThere's a missing semicolon after the declaration of
contentLength.Here's the fix:
- const contentLength = contentForRender.get('length') + const contentLength = contentForRender.get('length');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- addon/components/advlimit-dialog-content.js (5 hunks)
- addon/components/flexberry-base-component.js (1 hunks)
- addon/components/flexberry-button.js (1 hunks)
- addon/components/flexberry-file.js (4 hunks)
- addon/components/flexberry-lookup.js (5 hunks)
- addon/components/flexberry-objectlistview.js (3 hunks)
- addon/components/groupedit-toolbar.js (2 hunks)
- addon/components/mobile/flexberry-file.js (1 hunks)
- addon/components/mobile/flexberry-objectlistview.js (1 hunks)
- addon/components/mobile/object-list-view.js (2 hunks)
- addon/components/object-list-view-row.js (1 hunks)
- addon/components/object-list-view.js (12 hunks)
- addon/components/olv-toolbar.js (17 hunks)
- addon/mixins/edit-form-controller-operations-indication.js (4 hunks)
- addon/mixins/flexberry-lookup-controller.js (2 hunks)
- addon/mixins/flexberry-objectlistview-route.js (2 hunks)
- addon/utils/routes-names-builder.js (1 hunks)
🧰 Additional context used
🪛 Biome
addon/components/object-list-view.js
[error] 573-573: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
addon/components/olv-toolbar.js
[error] 19-19: Shouldn't redeclare 'getOwner'. Consider to delete it or rename it.
'getOwner' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (24)
addon/utils/routes-names-builder.js (2)
1-5: LGTM! Good separation of concerns.The function successfully centralizes the route name generation logic, making it more maintainable and consistent across the application.
1-5: Verify the impact of this change.Let's verify how this utility is being used across the codebase to ensure all use cases are covered.
✅ Verification successful
Let me gather more context about how this utility is being used in the files that import it.
Based on the comprehensive analysis of the codebase, I can now provide a final response:
The utility function is properly used and its changes are safe
The
getNewRouteNameOfEditFormutility function is consistently used across three components:
flexberry-objectlistview.jsflexberry-simpleolv.jsolv-toolbar.jsIn all cases, it's used to construct the route name for new form transitions by appending '.new' to the base route name. This matches the established routing pattern seen throughout the codebase where new form routes consistently follow the pattern
{base-route}.newwith corresponding path{base-route}/new.The implementation:
export default function getNewRouteNameOfEditForm(routeName) { const postfix = '.new'; return `${routeName}${postfix}`; }maintains this convention and is used correctly in all its implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct string concatenation with '.new' that could be replaced with this utility rg -g '!routes-names-builder.js' "\.new['\"]\s*\+" || rg -g '!routes-names-builder.js' "\.new\s*\+" # Search for existing imports of this utility to verify proper adoption rg "import.*getNewRouteNameOfEditForm.*from.*routes-names-builder"Length of output: 513
Script:
#!/bin/bash # Check usage in flexberry-objectlistview.js rg -A 5 "getNewRouteNameOfEditForm" addon/components/flexberry-objectlistview.js # Check usage in flexberry-simpleolv.js rg -A 5 "getNewRouteNameOfEditForm" addon/components/flexberry-simpleolv.js # Check usage in olv-toolbar.js rg -A 5 "getNewRouteNameOfEditForm" addon/components/olv-toolbar.js # Also search for any other occurrences of '.new' in route context rg -g "!*.{json,md,txt}" "\.new" -C 3Length of output: 35659
addon/components/mobile/flexberry-file.js (1)
39-39: Translation changes look good!The conversion from
this.get('i18n').t()tot()macro is consistent and maintains the same translation keys.Also applies to: 47-47, 55-55, 62-62, 69-69
addon/mixins/flexberry-objectlistview-route.js (1)
73-73: LGTM! Good localization improvementThe switch to using the translation macro (
t) instead of direct i18n service calls is a good practice that aligns with ember-i18n's recommendations and improves consistency across the codebase.addon/components/advlimit-dialog-content.js (3)
5-5: LGTM! Good improvement in localization handling.The introduction of
translationMacrois a positive change that aligns with Ember's best practices for internationalization, reducing boilerplate code and improving maintainability.
87-89: LGTM! Consistent error message localization.The conversion of error messages to use the translation macro is thorough and consistent throughout the component.
Also applies to: 108-108, 126-126
59-59: Verify consistency of translation keys across components.Let's ensure the translation keys follow the same pattern across all components that have been updated as part of this refactoring effort.
Also applies to: 87-87, 89-89, 102-102, 108-108, 118-118, 120-120, 126-126
✅ Verification successful
Based on the shell script results, I can now generate the final response as we have sufficient information to verify the translation key consistency.
Translation keys follow consistent naming pattern across components
The search results show that the translation keys in
advlimit-dialog-content.jsfollow a consistent pattern:
- All keys are prefixed with
components.advlimit-dialog-content.- The keys are used consistently across multiple files (
advlimit-dialog.js,olv-toolbar.js,advlimit-dialog-content.js)- The translation macro
tis imported correctly from 'ember-i18n' and used consistentlyThe translation keys follow the same hierarchical pattern used throughout the codebase, where other components also use the pattern
components.<component-name>.<key>.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all translation keys used in components to verify consistency rg -g '*.js' "components\.advlimit-dialog-content\." --type js # Search for other components using translationMacro to compare patterns rg -g '*.js' "import.*translationMacro.*from 'ember-i18n'" -A 5 --type jsLength of output: 28019
addon/components/mobile/object-list-view.js (1)
10-10: LGTM! Import of translationMacro follows conventions.The addition of the translationMacro import aligns with the PR's localization improvements and follows common Ember conventions.
addon/mixins/edit-form-controller-operations-indication.js (1)
113-118: Same modernization applies hereThe same modernization suggestions apply to this computed property as discussed above.
addon/components/flexberry-base-component.js (1)
253-257: Verify the method usage across components.The method appears to be used by several components. Let's verify its usage to ensure all implementations are updated.
✅ Verification successful
Based on the search results, I can now provide a final response:
Method
isNameOfCurrentComponentis actively used across the codebaseThe verification confirms that the method is indeed used in several components:
flexberry-lookup.js(3 usages)groupedit-toolbar.js(2 usages)flexberry-objectlistview.js(1 usage)olv-toolbar.js(5 usages)object-list-view.js(8 usages)flexberry-simpleolv.js(10 usages)Additionally, no direct component name comparisons were found, indicating that all components are properly using this method for component name checks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of isNameOfCurrentComponent across components # Expected: Find calls in the components mentioned in the summary echo "Searching for direct usage of isNameOfCurrentComponent..." rg "isNameOfCurrentComponent\(" addon/components/ echo -e "\nSearching for potential places where component name comparison is still done directly..." rg "componentName\s*===?\s*['\"]" addon/components/Length of output: 3451
addon/components/groupedit-toolbar.js (1)
295-298: LGTM!The change to use
isNameOfCurrentComponentimproves code consistency and maintainability.addon/components/object-list-view-row.js (1)
282-292: Verify custom button usage patterns.Let's ensure this refactoring doesn't introduce breaking changes for existing custom button implementations.
✅ Verification successful
Let me gather more information about the usage patterns and implementation details to better understand the impact.
Let me try one more search pattern to find concrete examples of custom button usage and their action definitions.
Based on the comprehensive search results, I can now provide a final response about the custom button action handler refactoring:
The action handler implementation is safe and consistent with existing usage patterns
The verification shows that:
- The switch statement handling both function and string action types is correctly implemented as it supports both:
- Direct function handlers (case 'function')
- Named actions that bubble up through
sendAction(case 'string')- This pattern is consistently used across the component hierarchy:
flexberry-objectlistviewandflexberry-groupeditcomponents both implement the same action bubbling pattern- Template usage shows both patterns are actively used in the codebase
- The error case for unsupported action types provides appropriate validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for custom button configurations and usages echo "Searching for custom button configurations..." rg -l "customButtonInRowAction" --type js echo -e "\nSearching for action parameter patterns..." # Look for places where actions are passed to check their types rg "customButtonInRowAction.*['\"]" --type js -A 2Length of output: 1190
Script:
#!/bin/bash # Check how customButtonInRowAction is handled in parent components echo "Checking customButtonInRowAction implementations in parent components..." rg "customButtonInRowAction.*function" -A 5 --type js echo -e "\nChecking custom button configurations..." # Look for custom button setup patterns rg "buttons.*=.*\[" -A 5 --type js echo -e "\nChecking for action parameter types..." # Look for places where action types are defined/used ast-grep --pattern 'action: $_'Length of output: 471
Script:
#!/bin/bash # Search for custom button definitions and their action configurations echo "Searching for button configurations in templates..." rg "customButtons" -B 2 -A 5 --type hbs echo -e "\nSearching for button action definitions in components..." rg "customButtons.*=" -B 2 -A 5 --type js echo -e "\nSearching for action handling in flexberry components..." rg "customButtonInRowAction" -B 2 -A 5 addon/components/flexberry-*.jsLength of output: 20663
addon/mixins/flexberry-lookup-controller.js (2)
327-327: LGTM: Good use of translation macroThe change from direct i18n service to translation macro is a good improvement as it will automatically update when the locale changes.
5-6:⚠️ Potential issueRemove unused Ember import
The
Emberimport appears to be unused as the code uses the newer@emberimports throughout. This aligns with modern Ember practices.Apply this diff to remove the unused import:
-import Ember from 'ember'; import { translationMacro as t } from 'ember-i18n';This issue was previously identified in a past review comment.
addon/components/flexberry-file.js (2)
975-976: LGTM! Consistent use of translationMacro for file size error messages.The change from i18n service to translationMacro (t) aligns with the component's existing translation patterns and improves consistency.
996-997: LGTM! Consistent use of translationMacro for file extension error messages.The change from i18n service to translationMacro (t) aligns with the component's existing translation patterns and improves consistency.
addon/components/olv-toolbar.js (3)
289-289: LGTM: Improved translation handling using macros.The replacement of direct
i18n.t()calls with the translation macrot()is a good improvement that:
- Enhances performance through computed translations
- Ensures automatic updates on locale changes
- Enables better static analysis
Also applies to: 298-298, 306-306, 331-331, 358-358, 402-402, 501-501, 773-773, 819-821, 888-888
961-961: LGTM: Improved component name validation.The use of
isNameOfCurrentComponenthelper method instead of direct component name comparisons:
- Makes the code more maintainable
- Reduces potential for errors
- Follows the DRY principle
Also applies to: 981-981, 988-988, 1039-1039, 1047-1047
538-540: Consider improving the route transition handling.The use of
Ember.run.laterwith a hardcoded 50ms delay could be problematic:
- It might lead to race conditions
- The purpose of the delay is not documented
- Uses a magic number
Consider either:
- Removing the delay if not strictly necessary
- Adding a comment explaining why the delay is needed
- Using a named constant for the delay value
addon/components/flexberry-objectlistview.js (2)
20-20: LGTM!The import statement is correctly placed and follows the standard import format.
1515-1515: LGTM!The use of
isNameOfCurrentComponentmethod improves code readability and maintainability by encapsulating the component name comparison logic.addon/components/flexberry-lookup.js (2)
1181-1183: LGTM: Translation macro changes improve i18n handling.The transition from direct i18n service usage to translation macros (
t) for message templates is a good improvement that follows Ember's best practices for internationalization.Also applies to: 1431-1431
1089-1089: LGTM: Component name checks are now more maintainable.The replacement of direct component name comparisons with
isNameOfCurrentComponentmethod improves code maintainability by:
- Centralizing the component name check logic
- Making the code more DRY by reusing the base component's functionality
Also applies to: 1103-1103, 1118-1118
addon/components/object-list-view.js (1)
2862-2862:⚠️ Potential issueTypo in variable name 'skipConfugureRows'
The variable
skipConfugureRowsseems to be misspelled. It should beskipConfigureRows.Apply this diff to fix the typo:
- if (!skipConfugureRows) { + if (!skipConfigureRows) {Likely invalid or redundant comment.
| export default function getNewRouteNameOfEditForm(routeName) { | ||
| const postfix = '.new'; | ||
|
|
||
| return `${routeName}${postfix}`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding input validation and documentation.
To improve robustness and maintainability:
- Add parameter validation
- Add JSDoc documentation describing the purpose, parameters, and return value
Here's a suggested implementation:
+/**
+ * Generates a new route name for edit form by appending '.new' to the base route name
+ * @param {string} routeName - The base route name
+ * @returns {string} The generated route name for new form
+ * @throws {Error} If routeName is null, undefined or not a string
+ */
export default function getNewRouteNameOfEditForm(routeName) {
+ if (!routeName || typeof routeName !== 'string') {
+ throw new Error('Route name must be a non-empty string');
+ }
+
const postfix = '.new';
return `${routeName}${postfix}`;
}📝 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.
| export default function getNewRouteNameOfEditForm(routeName) { | |
| const postfix = '.new'; | |
| return `${routeName}${postfix}`; | |
| } | |
| /** | |
| * Generates a new route name for edit form by appending '.new' to the base route name | |
| * @param {string} routeName - The base route name | |
| * @returns {string} The generated route name for new form | |
| * @throws {Error} If routeName is null, undefined or not a string | |
| */ | |
| export default function getNewRouteNameOfEditForm(routeName) { | |
| if (!routeName || typeof routeName !== 'string') { | |
| throw new Error('Route name must be a non-empty string'); | |
| } | |
| const postfix = '.new'; | |
| return `${routeName}${postfix}`; | |
| } |
| @@ -4,6 +4,8 @@ | |||
|
|
|||
| import Mixin from '@ember/object/mixin'; | |||
| import { merge } from '@ember/polyfills'; | |||
| import Ember from 'ember'; | |||
There was a problem hiding this comment.
Remove unnecessary Ember import
The global Ember import is not needed as the code already uses modular imports (e.g., @ember/object/mixin). This aligns with modern Ember.js best practices.
-import Ember from 'ember';📝 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.
| import Ember from 'ember'; |
| t('components.advlimit-dialog-content.limit') + | ||
| advLimitName + | ||
| this.get('i18n').t('components.advlimit-dialog-content.is-saved') | ||
| t('components.advlimit-dialog-content.is-saved') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using template literals for better readability.
The string concatenation could be simplified using template literals.
- t('components.advlimit-dialog-content.limit') +
- advLimitName +
- t('components.advlimit-dialog-content.is-saved')
+ `${t('components.advlimit-dialog-content.limit')}${advLimitName}${t('components.advlimit-dialog-content.is-saved')}`📝 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.
| t('components.advlimit-dialog-content.limit') + | |
| advLimitName + | |
| this.get('i18n').t('components.advlimit-dialog-content.is-saved') | |
| t('components.advlimit-dialog-content.is-saved') | |
| `${t('components.advlimit-dialog-content.limit')}${advLimitName}${t('components.advlimit-dialog-content.is-saved')}` |
| @@ -98,14 +98,14 @@ export default FlexberryBaseComponent.extend({ | |||
| saveAdvLimit: function() { | |||
| this._hideMessage(); | |||
| const advLimitName = this.get('model.advLimitName'); | |||
| if (isBlank(advLimitName)) { | |||
| this._showMessage('warning', this.get('i18n').t('components.advlimit-dialog-content.enter-setting-name')); | |||
| if (Ember.isBlank(advLimitName)) { | |||
There was a problem hiding this comment.
Use imported isBlank instead of Ember.isBlank.
Since isBlank is already imported from @ember/utils, use it directly instead of accessing it through the Ember namespace.
- if (Ember.isBlank(advLimitName)) {
+ if (isBlank(advLimitName)) {📝 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.
| if (Ember.isBlank(advLimitName)) { | |
| if (isBlank(advLimitName)) { |
| _hasCaption: Ember.computed('caption', function () { | ||
| const caption = this.get('caption'); | ||
|
|
||
| if (Ember.isEmpty(caption)) return false; | ||
|
|
||
| const isCaptionTypeIsString = Ember.typeOf(caption) === 'string'; | ||
| const isCaptionNotEmpty = Ember.$.trim(caption) !== ''; | ||
| const isCaptionSafe = Ember.String.isHTMLSafe(caption); | ||
| const isHTMLSafeDefined = Ember.typeOf(Ember.String.isHTMLSafe) === 'function'; | ||
| const isStringOfCaptionEmpty = Ember.$.trim(Ember.get(caption, 'string')) === ''; | ||
|
|
||
| return isCaptionTypeIsString && isCaptionNotEmpty | ||
| || isHTMLSafeDefined && isCaptionSafe && isStringOfCaptionEmpty === false | ||
| || caption instanceof Ember.Handlebars.SafeString && isStringOfCaptionEmpty === false; |
There was a problem hiding this comment.
Refactor to use imports and optimize the implementation.
Several improvements can be made to enhance maintainability and consistency:
- Use the imported
computedinstead ofEmber.computed - Replace direct
Ember.references with imports - Optimize the string checks
Apply these changes:
- _hasCaption: Ember.computed('caption', function () {
- const caption = this.get('caption');
-
- if (Ember.isEmpty(caption)) return false;
-
- const isCaptionTypeIsString = Ember.typeOf(caption) === 'string';
- const isCaptionNotEmpty = Ember.$.trim(caption) !== '';
- const isCaptionSafe = Ember.String.isHTMLSafe(caption);
- const isHTMLSafeDefined = Ember.typeOf(Ember.String.isHTMLSafe) === 'function';
- const isStringOfCaptionEmpty = Ember.$.trim(Ember.get(caption, 'string')) === '';
-
- return isCaptionTypeIsString && isCaptionNotEmpty
- || isHTMLSafeDefined && isCaptionSafe && isStringOfCaptionEmpty === false
- || caption instanceof Ember.Handlebars.SafeString && isStringOfCaptionEmpty === false;
+ _hasCaption: computed('caption', function() {
+ const caption = get(this, 'caption');
+
+ if (isEmpty(caption)) {
+ return false;
+ }
+
+ const trimmedCaption = $.trim(caption);
+
+ if (typeOf(caption) === 'string') {
+ return trimmedCaption !== '';
+ }
+
+ if (isHTMLSafe(caption)) {
+ return $.trim(get(caption, 'string')) !== '';
+ }
+
+ return false;
+ }),Add these imports at the top:
+ import { isEmpty } from '@ember/utils';Committable suggestion was skipped due to low confidence.
| @@ -14,6 +14,9 @@ import { getOwner } from '@ember/application'; | |||
| import { A } from '@ember/array'; | |||
| import FlexberryBaseComponent from './flexberry-base-component'; | |||
| import serializeSortingParam from '../utils/serialize-sorting-param'; | |||
| import { getNewRouteNameOfEditForm } from '../utils/routes-names-builder'; | |||
| import { translationMacro as t } from 'ember-i18n'; | |||
| const { getOwner } = Ember; | |||
There was a problem hiding this comment.
Remove redundant getOwner declaration.
The getOwner is already imported at line 9. This redeclaration is unnecessary and could lead to confusion.
-const { getOwner } = Ember;📝 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.
| const { getOwner } = Ember; |
🧰 Tools
🪛 Biome
[error] 19-19: Shouldn't redeclare 'getOwner'. Consider to delete it or rename it.
'getOwner' is defined here:
(lint/suspicious/noRedeclare)
| Ember.run.later((function() { | ||
| modelController.transitionToRoute(getNewRouteNameOfEditForm(editFormRoute), transitionOptions); |
There was a problem hiding this comment.
Remove unnecessary timer for route transition.
The use of Ember.run.later with a hardcoded 50ms delay is not recommended as it:
- Introduces a magic number
- Could lead to race conditions
- Makes the code harder to test
Consider removing the timer and transitioning directly:
- Ember.run.later((function() {
- modelController.transitionToRoute(getNewRouteNameOfEditForm(editFormRoute), transitionOptions);
- }), 50);
+ modelController.transitionToRoute(getNewRouteNameOfEditForm(editFormRoute), transitionOptions);Committable suggestion was skipped due to low confidence.
| let allWords = this.get('filterByAllWords'); | ||
| assert(`Only one of the options can be used: 'filterByAnyWord' or 'filterByAllWords'.`, !(allWords && anyWord)); | ||
| if (this.isNameOfCurrentComponent(componentName)) { | ||
| Ember.assert(`Only one of the options can be used: 'filterByAnyWord' or 'filterByAllWords'.`, !(allWords && anyWord)); |
There was a problem hiding this comment.
Undefined variables 'anyWord' and 'allWords' in assertion
Variables anyWord and allWords are not defined in this scope, which will cause a ReferenceError.
Please define anyWord and allWords, or remove the assertion if these variables are not needed.
| componentSettings.sorting.forEach((sorting, sortingIndex) => { | ||
| if (cachedColumn[sorting.propName]) { | ||
| cachedColumn[sorting.propName].sorted = true; | ||
| cachedColumn[sorting.propName].sortAscending = sorting.direction === 'asc' ? true : false; |
There was a problem hiding this comment.
Simplify boolean assignment in 'sortAscending'
You can simplify the assignment of sortAscending by directly using the comparison result without the ternary operator.
Apply this diff to fix the issue:
- cachedColumn[sorting.propName].sortAscending = sorting.direction === 'asc' ? true : false;
+ cachedColumn[sorting.propName].sortAscending = sorting.direction === 'asc';📝 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.
| cachedColumn[sorting.propName].sortAscending = sorting.direction === 'asc' ? true : false; | |
| cachedColumn[sorting.propName].sortAscending = sorting.direction === 'asc'; |
🧰 Tools
🪛 Biome
[error] 573-573: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
| 'eq': locale.eq, | ||
| 'neq': locale.neq, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Remove extraneous return statement at the end of '_conditionsByType'
There is an unnecessary return get(model, 'attributes').get(attributeName); statement at the end of the _conditionsByType method, which uses undefined variables and does not align with the method's purpose.
Remove the extraneous return statement:
- return get(model, 'attributes').get(attributeName);Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
Release Notes
New Features
translationMacro.isNameOfCurrentComponentto enhance component name checks.Bug Fixes
Refactor
Ember.computedfor consistency.Chores