upgrade rules page #704
upgrade rules page #704
Conversation
WalkthroughThis pull request updates the application by releasing version 2.111.0 of the package with new rule management features, including the ability to include, copy, and rename rules. It removes several script editor components and controllers while migrating AngularJS states to a component-based approach. New React directives and components for rule editing and code editing using CodeMirror are added. Localization for rules has been enhanced in both English and Russian, and various style, ESLint, and utility updates have been introduced to support the changes. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant ERP as EditRulePage
participant RS as RulesStore
participant CE as CodeEditor
participant NT as Notification System
U->>ERP: Open rule editing page
ERP->>RS: Request rule load (using current URL)
RS-->>ERP: Return rule content/errors
ERP->>CE: Render CodeEditor with rule content
U->>ERP: Modify rule via CodeEditor
U->>ERP: Click Save
ERP->>RS: Invoke save(rule)
RS-->>ERP: Return save status
ERP->>NT: Display notification based on result
NT-->>U: Show success/error message
Suggested reviewers
Poem
✨ 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: 30
🔭 Outside diff range comments (3)
frontend/src/components/cell/cell-value.tsx (1)
96-96: 🛠️ Refactor suggestionConsider safer alternatives to dangerouslySetInnerHTML.
The use of
dangerouslySetInnerHTMLfor space formatting could be replaced with a safer CSS-based solution.Here's a suggested implementation using CSS:
-<span dangerouslySetInnerHTML={{ __html: formattedValue }}></span> +<span className="deviceCell-value-formatted"> + {formattedValue.split(' ').map((part, index) => ( + <React.Fragment key={index}> + {index > 0 && <span className="deviceCell-space" />} + {part} + </React.Fragment> + ))} +</span> /* Add to CSS */ +.deviceCell-space { + display: inline-block; + width: 0.25em; +}🧰 Tools
🪛 Biome (1.9.4)
[error] 96-96: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 96-96: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
frontend/src/utils/url.ts (1)
1-5: 🧹 Nitpick (assertive)Handle empty or malformed hashes gracefully.
If
url.hashis empty or missing slashes,.pop()may returnundefined. WhiledecodeURIComponent('')is not harmful, consider a fallback:-return decodeURIComponent(url.hash.split('/').pop()); +return decodeURIComponent(url.hash.split('/').pop() ?? '');This ensures a safe default return value.
frontend/app/scripts/react-directives/rule/rule.tsx (1)
1-30: 🛠️ Refactor suggestionConsider extracting shared directive logic.
This file contains significant code duplication with
rules.tsx. Consider creating a shared higher-order directive or utility function.// shared-directive.ts export const createReactDirective = (Component, options = {}) => { return (whenMqttReady, EditorProxy, rolesFactory) => { 'ngInject'; setReactLocale(); return { restrict: 'E', scope: { path: '=', isNew: '=', ...options.scope, }, link: function(scope, element) { const hasRights = rolesFactory.checkRights(rolesFactory.ROLE_THREE); scope.store = new RulesStore(whenMqttReady, EditorProxy); scope.root = ReactDOM.createRoot(element[0]); scope.root.render(<Component rulesStore={scope.store} hasRights={hasRights} />); element.on('$destroy', () => { scope.store.dispose && scope.store.dispose(); scope.root.unmount(); }); }, ...options.directiveOptions, }; }; }; // Usage in rule.tsx and rules.tsx export default createReactDirective(EditRulePage);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (4)
frontend/src/assets/icons/copy.svgis excluded by!**/*.svgfrontend/src/assets/icons/edit.svgis excluded by!**/*.svgfrontend/src/assets/icons/success.svgis excluded by!**/*.svgfrontend/src/assets/icons/warn.svgis excluded by!**/*.svg
📒 Files selected for processing (50)
debian/changelog(1 hunks)frontend/app/scripts/app.js(0 hunks)frontend/app/scripts/app.routes.js(3 hunks)frontend/app/scripts/controllers/ruleController.js(1 hunks)frontend/app/scripts/controllers/rulesController.js(1 hunks)frontend/app/scripts/controllers/scriptController.js(0 hunks)frontend/app/scripts/controllers/scriptsController.js(0 hunks)frontend/app/scripts/i18n/react/locales/en/translations.json(1 hunks)frontend/app/scripts/i18n/react/locales/ru/translations.json(1 hunks)frontend/app/scripts/i18n/rules/en.json(0 hunks)frontend/app/scripts/i18n/rules/ru.json(0 hunks)frontend/app/scripts/react-directives/rule/rule.tsx(1 hunks)frontend/app/scripts/react-directives/rules/rules.tsx(1 hunks)frontend/app/scripts/react-directives/script-editor/pageStore.js(0 hunks)frontend/app/scripts/react-directives/script-editor/script-editor.js(0 hunks)frontend/app/scripts/react-directives/script-editor/scriptEditor.jsx(0 hunks)frontend/app/scripts/react-directives/script-editor/scriptEditorPage.jsx(0 hunks)frontend/app/styles/css/script-editor-page.css(0 hunks)frontend/app/views/script.html(0 hunks)frontend/app/views/scripts.html(0 hunks)frontend/eslint.config.mjs(1 hunks)frontend/src/assets/styles/variables.css(2 hunks)frontend/src/components/button/button.tsx(1 hunks)frontend/src/components/button/styles.css(3 hunks)frontend/src/components/button/types.ts(1 hunks)frontend/src/components/cell/cell-text.tsx(1 hunks)frontend/src/components/cell/cell-value.tsx(1 hunks)frontend/src/components/code-editor/code-editor.tsx(1 hunks)frontend/src/components/code-editor/helpers.ts(1 hunks)frontend/src/components/code-editor/index.ts(1 hunks)frontend/src/components/code-editor/styles.css(1 hunks)frontend/src/components/code-editor/types.ts(1 hunks)frontend/src/components/input/input.tsx(2 hunks)frontend/src/components/input/styles.css(1 hunks)frontend/src/components/input/types.ts(1 hunks)frontend/src/components/switch/switch.tsx(1 hunks)frontend/src/layouts/page/page.tsx(1 hunks)frontend/src/layouts/page/styles.css(2 hunks)frontend/src/layouts/page/types.ts(1 hunks)frontend/src/pages/rules/[rule]/edit-rule.tsx(1 hunks)frontend/src/pages/rules/[rule]/index.ts(1 hunks)frontend/src/pages/rules/[rule]/styles.css(1 hunks)frontend/src/pages/rules/index/index.ts(1 hunks)frontend/src/pages/rules/index/rules.tsx(1 hunks)frontend/src/pages/rules/index/styles.css(1 hunks)frontend/src/stores/rules/index.ts(1 hunks)frontend/src/stores/rules/rules-store.ts(1 hunks)frontend/src/stores/rules/types.ts(1 hunks)frontend/src/utils/id.ts(1 hunks)frontend/src/utils/url.ts(1 hunks)
💤 Files with no reviewable changes (12)
- frontend/app/views/script.html
- frontend/app/scripts/i18n/rules/en.json
- frontend/app/styles/css/script-editor-page.css
- frontend/app/scripts/app.js
- frontend/app/scripts/i18n/rules/ru.json
- frontend/app/views/scripts.html
- frontend/app/scripts/react-directives/script-editor/scriptEditor.jsx
- frontend/app/scripts/react-directives/script-editor/script-editor.js
- frontend/app/scripts/controllers/scriptController.js
- frontend/app/scripts/react-directives/script-editor/scriptEditorPage.jsx
- frontend/app/scripts/react-directives/script-editor/pageStore.js
- frontend/app/scripts/controllers/scriptsController.js
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/components/switch/switch.tsx
[error] 7-13: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
frontend/src/utils/id.ts
[error] 9-9: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
frontend/src/pages/rules/[rule]/edit-rule.tsx
[error] 59-59: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 62-62: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
frontend/src/components/code-editor/helpers.ts
[error] 18-18: This let declares a variable that is only assigned once.
'e' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 35-35: This let declares a variable that is only assigned once.
'breakpoints' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
frontend/src/layouts/page/page.tsx
[error] 83-83: Provide screen reader accessible content when using a elements.
All links on a page should have content that is accessible to screen readers.
Accessible content refers to digital content that is designed and structured in a way that makes it easy for people with disabilities to access, understand, and interact with using assistive technologies.
Follow these links for more information,
WCAG 2.4.4
WCAG 4.1.2
(lint/a11y/useAnchorContent)
[error] 102-102: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (51)
frontend/src/components/input/types.ts (1)
14-15: LGTM! Props are well-typed and follow React naming conventions.The new optional props
isFullWidthandisWithExplicitChangesare appropriately typed as booleans and follow React naming conventions.frontend/src/components/cell/cell-text.tsx (1)
42-42: LGTM! Good use of explicit change handling.The
isWithExplicitChangesprop is appropriately used here to ensure controlled input behavior.frontend/src/components/cell/cell-value.tsx (1)
69-69: LGTM! Good use of explicit change handling for numeric inputs.The
isWithExplicitChangesprop is appropriately used here to ensure controlled numeric input behavior.frontend/src/components/input/styles.css (1)
21-25: Added full-width style for inputs.
The new.input-fullWidthclass effectively sets the width to 100%, which should enhance the flexibility of input elements when theisFullWidthprop is enabled in the corresponding component. Ensure that the usage of this class is consistent across the application and aligns with your design system’s naming conventions.frontend/src/components/code-editor/helpers.ts (2)
4-5: The breakpoint effect approach looks clean.Defining a separate
StateEffectkeeps breakpoint toggling modular and composable. This clearly communicates the toggle intent within the editor state.
58-79: Validate error lines or provide fallback.If an error line exceeds the total number of lines, it simply returns. This is safe. However, if
.pop()inside line-based logic returnsundefined, ensure it won't cause unintended side effects. Otherwise, the approach is solid.frontend/src/layouts/page/page.tsx (5)
14-24: Prop expansions are flexible and descriptive.Allowing
errors,stickyHeader, and editing-related props provides robust customizability. Good job specifying defaults likeerrors = []andisLoading = false.
25-32: Properly initializing state from props.Using
useEffectto synctitleValuewithtitleand a separate state forisEditingTitleStateis a nice pattern. This ensures you won't override user input unexpectedly.
33-44: 404 handling block is clear and user-friendly.Displaying the not-found message with an alert is straightforward.
73-77: Meaningful error alerts.Mapping over
errorsto display each alert by variant is concise. Confirm that theerror.textis safe for display (e.g., sanitized, if user-input).
88-100: Conditional rendering of loader and content.The
stickyHeaderusage is intuitive. Users can see a loader ifisLoadingis true; otherwise, the main content is displayed. This pattern is flexible and clean.frontend/app/scripts/controllers/ruleController.js (1)
1-5:✅ Verification successful
LGTM! Verify dependency availability.
The implementation correctly bridges AngularJS and React components. The directive is properly registered with necessary dependencies.
Run the following script to verify the dependency availability:
🏁 Script executed:
#!/bin/bash # Description: Verify that all required dependencies are available. # Check for MQTT service rg -A 5 'whenMqttReady' # Check for EditorProxy service rg -A 5 'EditorProxy' # Check for rolesFactory service rg -A 5 'rolesFactory' # Check for React directive rg -A 5 'react-directives/rule/rule'Length of output: 74307
Dependency Verification and Approval
The dependency checks confirm that all required services—
whenMqttReady,EditorProxy, androlesFactory—are properly declared and used throughout the codebase, and the React directive import fromreact-directives/rule/rule.tsxis consistent with the expected usage in the AngularJS module registration.No further changes are needed.
frontend/src/components/button/types.ts (1)
5-7: LGTM! Enhanced button flexibility.The changes improve the button component by:
- Making the label optional to support icon-only buttons
- Adding icon support for better visual representation
- Including a success variant for positive actions
frontend/src/layouts/page/types.ts (1)
10-10: LGTM!The property name is self-descriptive and the type is appropriate.
frontend/src/components/button/button.tsx (2)
6-6: LGTM! Props destructuring follows React best practices.The props destructuring is clean and follows React best practices by providing default values for
sizeandvariant.
14-14: LGTM! Success variant class follows naming convention.The success variant class follows the established naming pattern of other variants.
frontend/app/scripts/react-directives/rules/rules.tsx (1)
7-9: LGTM! Proper AngularJS directive setup with dependency injection.The directive follows AngularJS best practices with proper dependency injection using 'ngInject'.
frontend/src/components/code-editor/code-editor.tsx (2)
12-25: LGTM! Proper error line handling and gutter effects.The editor reinitialization logic properly handles error lines and gutter effects.
52-62: LGTM! Clean component rendering with proper props.The CodeMirror component is rendered with all necessary props and proper style handling.
frontend/src/stores/rules/rules-store.ts (1)
23-40: LGTM! Well-structured rule loading logic.The implementation properly handles asynchronous loading, error states, and MobX state updates.
frontend/src/pages/rules/[rule]/edit-rule.tsx (1)
30-45: LGTM! Well-structured initialization and error handling.The component properly manages loading states and handles errors during rule loading.
frontend/eslint.config.mjs (1)
72-73: LGTM! Good addition of JSX formatting rules.The new rules will help maintain consistent JSX formatting across the codebase.
frontend/app/scripts/app.routes.js (3)
171-182: Migration to component-based architecture for rules page.The rules state has been updated to use a component-based approach with
<rules-page />instead of the previous template and controller pattern.
184-195: Migration to component-based architecture for rule editing.The rules-edit state has been updated to use a component-based approach with
<rule-page />. The chunk name has been changed from 'rules' to 'rule' to better reflect its purpose.
197-208: Migration to component-based architecture for new rule creation.The rules-new state has been updated to use a component-based approach with
<rule-page />, maintaining consistency with the rules-edit state.debian/changelog (1)
1-5: LGTM! Clear and concise changelog entry.The version bump to 2.111.0 is appropriate for the new feature addition. The changelog entry clearly describes the new functionality for rules management.
frontend/src/components/code-editor/index.ts (1)
1-2: LGTM! Clean barrel file implementation.The file follows the standard pattern for barrel files, providing a clean export of the CodeEditor component.
frontend/src/pages/rules/index/index.ts (1)
1-4: LGTM! Clean barrel file implementation.The file follows the standard pattern for barrel files, providing a clean default export of the RulesPage component.
frontend/src/pages/rules/[rule]/index.ts (1)
1-3: Simple and Clear Default ExportThe file cleanly imports the
EditRulePagecomponent and re-exports it as the default export. This straightforward approach makes the module easy to consume and understand.frontend/src/stores/rules/index.ts (1)
1-6: Centralizing the RulesStore ExportThe file correctly imports the
RulesStorefrom therules-storemodule and re-exports it. This central index file improves module accessibility and maintainability.frontend/src/components/code-editor/styles.css (1)
1-6: Appropriate Styling for the Code Editor GuttersThe new CSS rule for
.codeEditor-gutters .cm-gutterElementis well-defined. The explicit settings (red color, padding, font size, and cursor) appear to directly address the UI enhancement goals for the code editor component.
Minor Note: Double-check that the red color aligns with the overall design and accessibility standards.frontend/src/pages/rules/[rule]/styles.css (1)
1-14: Well-Structured Styles for the EditRulePageThe CSS rules for
.editRule-container,.editRule-nameInput, and.editRule-errorare clear and concise, providing the necessary stylistic guidelines for the rule editing interface.
Suggestion: Ensure that these styles integrate seamlessly with other page layouts, especially when used alongside global CSS variables or themes.frontend/src/pages/rules/index/styles.css (1)
1-41: Comprehensive Styling for the Rules List ComponentsThe CSS file provides a well-organized set of styles for the rules list items. The grid layout, hover/focus effects, and icon visibility settings demonstrate a thoughtful approach to interactive UI components.
Minor Suggestion: Verify that the usage of properties likefit-content(30px)is fully supported by your target browsers, or consider providing fallbacks if necessary. Also, ensure that all CSS variables (e.g.,--border-color,--table-item-hover,--text-color, and--warn-color) are defined globally.frontend/src/layouts/page/styles.css (6)
1-6: Good Implementation for the Page Container.
The new.pageclass sets the height and width to 100% and defines a flex column layout, which is appropriate for a full-page container.
13-21: Improved Header Container Spacing.
The modifications in.page-headerContainer—adjusting the margin on line 18 and adding a gap of 12px on line 20—enhance the spacing between header elements and align with modern flexbox layout practices.
23-29: Clear and Consistent Header Title Wrapper.
The newly introduced.page-headerTitleWrapperclass effectively uses flexbox with a gap and fixed height. Ensure the fixed height (30px) meets all design requirements for varying content sizes, although it is acceptable if the design is fixed.
31-35: Icon for Editing Title is Appropriately Styled.
The.page-editTitleIconclass sets a proper width, inline-block display, and a pointer cursor, which is ideal for clickable icons.
41-48: Scrollable Container for Page Content.
The newly added.page-containerclass combines max-height with overflow management and a flex column layout. This design will support scrollable content within a flexible container effectively.
50-52: Spacing for Error Messages is Clear.
The.page-errorclass provides a bottom margin for error messages, ensuring that error blocks do not crowd other content.frontend/src/components/button/styles.css (6)
8-10: Disabled State Visual Feedback.
The new rule for.button:disabledapplies an opacity filter, which is a simple and effective method to indicate a disabled button.
28-30: Refined Hover State for Primary Buttons.
Updating the hover selector to.button-primary:not(:disabled):hoverensures that hover effects only occur for enabled buttons, thereby improving user experience.
38-40: Consistent Hover Feedback for Secondary Buttons.
The analogous change for.button-secondary:not(:disabled):hoveraligns with the primary button adjustments and ensures consistent behavior across button variants.
42-44: Introduction of the Success Button Style.
The new.button-successclass adds a success variant with its background set tovar(--success-color). This provides an additional style option for actions that are contextually “successful.”
46-48: Hover State for Success Buttons.
The rule.button-success:not(:disabled):hoverensures that the hover state is applied correctly only when the button is active, usingvar(--success-color-hover).
57-67: Icon Button Enhancements.
The addition of.button-iconand its child rule for SVG elements (ensuring dimensions scale to 100%) are well implemented. This setup will help maintain consistent icon sizing within buttons.frontend/src/assets/styles/variables.css (2)
12-12: Addition of Warning Color Variable.
Defining--warn-color: #eebc40;enriches the color palette for warning states. Verify that this value integrates well with the overall design language.
66-67: New Table Hover Variable Introduced.
Adding--table-item-hover: #fbfbfb;provides a subtle hover effect for table items, enhancing visual feedback in table interactions.frontend/app/scripts/i18n/react/locales/en/translations.json (2)
438-462: Comprehensive Addition of the Rules Section.
The new"rules"section introduces all the necessary keys (title, buttons, labels, prompt, errors) for rule management. The structure is clear and consistent with the rest of the localization file.
463-467: Enhanced Page Not Found Messages.
The"page"section now includes"not-found"and"not-found-description"keys. The English messages are concise and inform users appropriately when a page is unavailable.frontend/app/scripts/i18n/react/locales/ru/translations.json (2)
464-488: Добавление секции "rules" в русской локализации.
Новая секция"rules"включает все необходимые ключи (заголовок, кнопки, метки, подтверждения, ошибки) для управления правилами. Текст полон и соответствует содержанию английской версии.
490-493: Улучшенные сообщения для ошибок 404.
В секции"page"добавлены ключи"not-found"и"not-found-description", сообщающие пользователю о том, что запрошенная страница не существует. Тексты понятны и корректны.
| return ( | ||
| <input | ||
| type={type} | ||
| className={classNames('input', className, { | ||
| 'input-m': size === 'default', | ||
| 'input-s': size === 'small', | ||
| 'input-fullWidth': isFullWidth, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using CSS-in-JS or CSS modules for better encapsulation.
The class name input-fullWidth follows BEM naming convention, but consider using CSS-in-JS or CSS modules to avoid potential naming conflicts.
| const handleOnChange = (ev: ChangeEvent<HTMLInputElement>): void => { | ||
| setInternalValue(ev.target.value); | ||
| if (!isWithExplicitChanges) { | ||
| onChange(ev.target.value); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding input validation and debouncing.
The new change handler logic looks good, but consider these improvements:
- Add input validation for numeric types.
- Consider debouncing frequent changes to prevent performance issues.
Here's a suggested implementation:
+import { useMemo } from 'react';
+import debounce from 'lodash/debounce';
const handleOnChange = (ev: ChangeEvent<HTMLInputElement>): void => {
+ const newValue = ev.target.value;
+
+ if (type === 'number' && !isValidNumber(newValue)) {
+ return;
+ }
+
setInternalValue(ev.target.value);
if (!isWithExplicitChanges) {
- onChange(ev.target.value);
+ debouncedOnChange(ev.target.value);
}
};
+const debouncedOnChange = useMemo(
+ () => debounce((value: string) => onChange(value), 300),
+ [onChange]
+);
+
+const isValidNumber = (value: string): boolean => {
+ const num = parseFloat(value);
+ return !isNaN(num) &&
+ (typeof min === 'undefined' || num >= min) &&
+ (typeof max === 'undefined' || num <= max);
+};📝 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 handleOnChange = (ev: ChangeEvent<HTMLInputElement>): void => { | |
| setInternalValue(ev.target.value); | |
| if (!isWithExplicitChanges) { | |
| onChange(ev.target.value); | |
| } | |
| }; | |
| import { useMemo } from 'react'; | |
| import debounce from 'lodash/debounce'; | |
| // ... other imports and component code | |
| const handleOnChange = (ev: ChangeEvent<HTMLInputElement>): void => { | |
| const newValue = ev.target.value; | |
| if (type === 'number' && !isValidNumber(newValue)) { | |
| return; | |
| } | |
| setInternalValue(ev.target.value); | |
| if (!isWithExplicitChanges) { | |
| debouncedOnChange(ev.target.value); | |
| } | |
| }; | |
| const debouncedOnChange = useMemo( | |
| () => debounce((value: string) => onChange(value), 300), | |
| [onChange] | |
| ); | |
| const isValidNumber = (value: string): boolean => { | |
| const num = parseFloat(value); | |
| return ( | |
| !isNaN(num) && | |
| (typeof min === 'undefined' || num >= min) && | |
| (typeof max === 'undefined' || num <= max) | |
| ); | |
| }; | |
| // ... rest of the component code |
| import { EditorState, RangeSet, StateEffect, StateField } from '@codemirror/state'; | ||
| import { EditorView, gutter, GutterMarker } from '@codemirror/view'; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider tree-shaking or partial imports.
If the bundle size is a concern, you could import only the required items from @codemirror/state and @codemirror/view to reduce final bundle size. Otherwise, this is a straightforward import statement.
| const breakpointMarker = new class extends GutterMarker { | ||
| toDOM() { | ||
| return document.createTextNode('⚠'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use an icon or distinct marker for breakpoints.
Currently, the marker is a text node '⚠'. Consider whether a custom icon or more descriptive marker would improve UX, particularly for visually impaired users or for color-blind friendly design.
| export const customGutter = gutter({ | ||
| class: 'codeEditor-gutters', | ||
| markers: (view) => view.state.field(breakpointState) || RangeSet.empty, | ||
| domEventHandlers: { | ||
| mousedown(view, line) { | ||
| toggleBreakpoint(view, line.from); | ||
| return true; | ||
| }, | ||
| }, | ||
| initialSpacer: () => null, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Well-defined custom gutter.
Attaching the mousedown handler to toggle breakpoints is a concise approach. Ensure you document or handle potential double-click scenarios if needed (e.g. toggling off and on quickly).
| #editorProxy: any; | ||
| #whenMqttReady: any; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding proper types for private fields.
The use of any type reduces type safety. Consider defining proper interfaces for these dependencies.
- #editorProxy: any;
- #whenMqttReady: any;
+ #editorProxy: EditorProxy;
+ #whenMqttReady: () => Promise<void>;📝 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.
| #editorProxy: any; | |
| #whenMqttReady: any; | |
| #editorProxy: EditorProxy; | |
| #whenMqttReady: () => Promise<void>; |
| async deleteRule(path: string) { | ||
| return this.#editorProxy.Remove({ path }).then((res: boolean) => { | ||
| if (res) { | ||
| runInAction(() => { | ||
| this.rules = this.rules.filter((rule) => rule.virtualPath !== path); | ||
| }); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider consistent error handling across operations.
The deleteRule method could benefit from similar error handling as implemented in the save method.
async deleteRule(path: string) {
- return this.#editorProxy.Remove({ path }).then((res: boolean) => {
+ try {
+ const res = await this.#editorProxy.Remove({ path });
if (res) {
runInAction(() => {
this.rules = this.rules.filter((rule) => rule.virtualPath !== path);
});
}
- });
+ return res;
+ } catch (error) {
+ throw new Error(error.data === 'MqttConnectionError'
+ ? 'MQTT connection error'
+ : error.message);
+ }
}📝 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.
| async deleteRule(path: string) { | |
| return this.#editorProxy.Remove({ path }).then((res: boolean) => { | |
| if (res) { | |
| runInAction(() => { | |
| this.rules = this.rules.filter((rule) => rule.virtualPath !== path); | |
| }); | |
| } | |
| }); | |
| } | |
| async deleteRule(path: string) { | |
| try { | |
| const res = await this.#editorProxy.Remove({ path }); | |
| if (res) { | |
| runInAction(() => { | |
| this.rules = this.rules.filter((rule) => rule.virtualPath !== path); | |
| }); | |
| } | |
| return res; | |
| } catch (error) { | |
| throw new Error(error.data === 'MqttConnectionError' | |
| ? 'MQTT connection error' | |
| : error.message); | |
| } | |
| } |
| } | ||
|
|
||
| async save(rule: Rule): Promise<string> { | ||
| const path = rule.name.endsWith('.js') ? rule.name : `/${rule.name}.js`; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider extracting path normalization logic.
The path normalization logic could be moved to a utility function for reusability and consistency.
+ private normalizePath(name: string): string {
+ return name.endsWith('.js') ? name : `/${name}.js`;
+ }
async save(rule: Rule): Promise<string> {
- const path = rule.name.endsWith('.js') ? rule.name : `/${rule.name}.js`;
+ const path = this.normalizePath(rule.name);📝 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 path = rule.name.endsWith('.js') ? rule.name : `/${rule.name}.js`; | |
| class RulesStore { | |
| // ... other members and methods | |
| private normalizePath(name: string): string { | |
| return name.endsWith('.js') ? name : `/${name}.js`; | |
| } | |
| async save(rule: Rule): Promise<string> { | |
| const path = this.normalizePath(rule.name); | |
| // ... rest of the save implementation | |
| } | |
| // ... other members and methods | |
| } |
| if (pathName === 'new') { | ||
| return location.replace('/#!/rules/edit/' + savedRuleName); | ||
| } else if (rule.initName !== savedRuleName) { | ||
| await rulesStore.deleteRule(rule.initName); | ||
| return location.replace('/#!/rules/edit/' + savedRuleName); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use template literals for URL construction.
Replace string concatenation with template literals for better readability.
- return location.replace('/#!/rules/edit/' + savedRuleName);
+ return location.replace(`/#!/rules/edit/${savedRuleName}`);
- return location.replace('/#!/rules/edit/' + savedRuleName);
+ return location.replace(`/#!/rules/edit/${savedRuleName}`);📝 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 (pathName === 'new') { | |
| return location.replace('/#!/rules/edit/' + savedRuleName); | |
| } else if (rule.initName !== savedRuleName) { | |
| await rulesStore.deleteRule(rule.initName); | |
| return location.replace('/#!/rules/edit/' + savedRuleName); | |
| } | |
| if (pathName === 'new') { | |
| return location.replace(`/#!/rules/edit/${savedRuleName}`); | |
| } else if (rule.initName !== savedRuleName) { | |
| await rulesStore.deleteRule(rule.initName); | |
| return location.replace(`/#!/rules/edit/${savedRuleName}`); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 59-59: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 62-62: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
| }); | ||
| }, []); | ||
|
|
||
| const createRule = () => location.assign('/#!/rules/new'); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using React Router for navigation.
Using location.assign directly is not ideal in a React application. Consider using React Router's navigation methods.
- const createRule = () => location.assign('/#!/rules/new');
+ const navigate = useNavigate();
+ const createRule = () => navigate('/rules/new');📝 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 createRule = () => location.assign('/#!/rules/new'); | |
| const navigate = useNavigate(); | |
| const createRule = () => navigate('/rules/new'); |
Обновил страницы правил и редактирования правил
Summary by CodeRabbit
New Features
Refactor
Style