Fix segment #139
Fix segment #139
Conversation
Reviewer's GuideThis PR refactors the segments and automations UI modules by consolidating GraphQL definitions into named exports, migrating query hooks to a cursor-based pagination hook, reorganizing and renaming form components (e.g., Property→SegmentProperty, Segment→SegmentGroup, Segments→SegmentGroups), replacing Combobox groups with Collapsible wrappers, extracting shared utilities, and cleaning up unused code/console logs. Sequence Diagram for Data Fetching in Attributes.tsx with useQuerySelectInputListsequenceDiagram
participant User
participant Attributes_Component as "Attributes.tsx"
participant useQuerySelectInputList_Hook as "useQuerySelectInputList"
participant ApolloClient as "Apollo Client"
participant GraphQL_Server as "GraphQL Server"
User->>Attributes_Component: Interacts (e.g., opens combobox)
Attributes_Component->>useQuerySelectInputList_Hook: Calls hook with query, queryName
useQuerySelectInputList_Hook->>ApolloClient: useQuery(query, variables)
ApolloClient->>GraphQL_Server: Executes GraphQL query (initial load)
GraphQL_Server-->>ApolloClient: Returns initial data (list, pageInfo)
ApolloClient-->>useQuerySelectInputList_Hook: Provides data and loading state
useQuerySelectInputList_Hook-->>Attributes_Component: Returns list, totalCount, loading, handleFetchMore
Attributes_Component->>Attributes_Component: Renders options based on list
alt User scrolls / requests more items
User->>Attributes_Component: Triggers fetch more
Attributes_Component->>useQuerySelectInputList_Hook: Calls handleFetchMore({ direction: "forward" })
useQuerySelectInputList_Hook->>ApolloClient: fetchMore(variables with new cursor)
ApolloClient->>GraphQL_Server: Executes GraphQL query (paginated)
GraphQL_Server-->>ApolloClient: Returns next page data
ApolloClient-->>useQuerySelectInputList_Hook: Provides updated data (merged list, new pageInfo)
useQuerySelectInputList_Hook-->>Attributes_Component: Returns updated list
Attributes_Component->>Attributes_Component: Renders more options
end
Class Diagram for Refactored Segment Form ComponentsclassDiagram
direction LR
class SegmentForm {
+contentType: string
+isTemporary: boolean
+callback: function
+segmentId: string
+SegmentForm()
+renderContent()
+handleCalculateStats()
+onSubmit()
}
class SegmentGroup {
+form: UseFormReturn
+parentFieldName: string
+onRemove: function
+contentType: string
+SegmentGroup()
}
class SegmentGroups {
+form: UseFormReturn
+contentType: string
+SegmentGroups()
}
class SegmentProperty {
+form: UseFormReturn
+index: number
+condition: IPropertyCondtion
+parentFieldName: string
+onRemove: function
+propertyTypes: object
+contentType: string
+SegmentProperty()
}
class QuerySelectInput {
+query: DocumentNode
+queryName: string
+labelField: string
+valueField: string
+onSelect()
+QuerySelectInput()
}
class SelectSegment {
+selected: string
+onSelect: function
+SelectSegment()
}
SegmentForm o-- "1" SegmentGroups
SegmentForm o-- "1" SegmentGroup
SegmentForm o-- "1" SelectSegment
SegmentGroups o-- "*" SegmentGroup
SegmentGroup o-- "*" SegmentProperty
SegmentProperty o-- "1" QuerySelectInput : uses
Class Diagram for Attributes.tsx Update with Collapsible UIclassDiagram
direction TB
class Attributes_Old {
+contentType: string
+selectedField: any
+onSelect: function
-renderSelection()
-renderOptions()
}
Attributes_Old --> useFieldSelectionList_OldHook : uses
Attributes_Old --> GraphQL_Queries_OldDefaultExport : uses
class Attributes_New {
+contentType: string
+selectedField: any
+onSelect: function
-renderSelection()
-renderOptions()
}
Attributes_New --> useQuerySelectInputList_NewHook : uses
Attributes_New --> GraphQL_Queries_NewNamedExport : uses
Attributes_New --> erxes_ui.Collapsible : uses
Attributes_New --> erxes_ui.Combobox : uses
Attributes_New --> erxes_ui.Command : uses
class useFieldSelectionList_OldHook
class useQuerySelectInputList_NewHook
class GraphQL_Queries_OldDefaultExport {
+fieldsCombinedByContentType
}
class GraphQL_Queries_NewNamedExport {
+FIELDS_COMBINED_BY_CONTENT_TYPE
}
Attributes_Old ..> Attributes_New : refactored
useFieldSelectionList_OldHook ..> useQuerySelectInputList_NewHook : replaced by
GraphQL_Queries_OldDefaultExport ..> GraphQL_Queries_NewNamedExport : refactored to named export
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThis update refactors the segments module, consolidating and renaming components, hooks, utilities, and GraphQL queries/mutations. It removes obsolete files, introduces new utility and input components, and restructures exports for clarity. The segments form and related UI now use new helpers and naming conventions, with enhanced type safety and more granular, explicit module exports. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SegmentsIndexPage
participant SegmentsRecordTable
participant SegmentListSidebar
participant QuerySelectInput
participant useQuerySelectInputList
participant GraphQL
User->>SegmentsIndexPage: Navigates to segments index
SegmentsIndexPage->>SegmentsRecordTable: Render table
SegmentsIndexPage->>SegmentListSidebar: Render sidebar
SegmentsRecordTable->>QuerySelectInput: Render select input
QuerySelectInput->>useQuerySelectInputList: Fetch options (debounced)
useQuerySelectInputList->>GraphQL: Query with search, cursor
GraphQL-->>useQuerySelectInputList: Return options, pageInfo
useQuerySelectInputList-->>QuerySelectInput: Provide options
QuerySelectInput-->>SegmentsRecordTable: User selects option
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 23f2aca in 2 minutes and 0 seconds. Click for details.
- Reviewed
2016lines of code in33files - Skipped
0files when reviewing. - Skipped posting
9draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/core-ui/src/modules/app/components/SegmentsRoutes.tsx:17
- Draft comment:
The detail route currently renders SegmentsIndexPage. Confirm if this is intentional or if a dedicated detail component (e.g. SegmentDetail) should be used. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is asking "Confirm if this is intentional" which directly violates our rule about not asking authors to confirm their intentions. The change appears deliberate - they explicitly removed SegmentsDetailPage and updated both routes. We should trust that the author made this change intentionally. Maybe this is actually a serious architectural issue that needs discussion, since having the same component for different routes could indicate a design problem? Even if it is an architectural issue, asking for confirmation of intention is not the right approach. If there's a concrete problem, it should be stated directly. Delete the comment because it violates our rule about not asking authors to confirm their intentions or explain their changes.
2. frontend/libs/ui-modules/src/modules/segments/components/form/SegmentForm.tsx:160
- Draft comment:
Both 'defaultValues' and 'values' are passed to useForm using getDefaultValues. Consider using only one to avoid potential state conflicts. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. frontend/libs/ui-modules/src/modules/segments/components/form/SegmentProperty.tsx:87
- Draft comment:
Avoid using array indices as keys when mapping lists; consider using unique identifiers (if available) for better rendering performance. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/libs/ui-modules/src/modules/segments/graphql/mutations.ts:10
- Draft comment:
Ensure the GraphQL query syntax in PARAMS_DEFS and PARAMS is correct. Consider adding explicit commas between arguments for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. frontend/plugins/automations_ui/src/modules/app/components/editor/actions/ManageProperties.tsx:227
- Draft comment:
Using the array index as a key when mapping over rules might lead to rendering issues. Consider using a stable unique identifier if available. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. frontend/core-ui/src/modules/app/components/SegmentsRoutes.tsx:12
- Draft comment:
There appears to be a naming inconsistency: the file is namedSegmentsRoutes.tsxwhile the exported component is namedSegmentRoutes. Consider renaming the export toSegmentsRoutesor, if intentional, adding a comment to clarify the choice. - Reason this comment was not posted:
Comment was on unchanged code.
7. frontend/libs/ui-modules/src/modules/automations/index.ts:2
- Draft comment:
Typographical issue: The filename 'PlaceHolderInput' might be a mistake. Typically 'PlaceholderInput' is used. Please verify the correct casing and spelling. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the capitalization is unconventional, this is purely a stylistic issue. The code will work fine either way. The comment is asking for verification rather than pointing out a clear issue. We don't have access to the actual component file to verify if this is even a typo - it could be intentionally named this way. The unusual capitalization could indicate a real typo that should be fixed for consistency. Bad naming conventions can make code harder to maintain. Even if it is a typo, the comment violates our rules by asking for verification rather than stating a clear issue. We also don't have enough context to be certain this is wrong. Delete this comment. It asks for verification rather than stating a clear issue, and we don't have enough context to be certain the capitalization is wrong.
8. frontend/libs/ui-modules/src/modules/segments/components/form/SegmentForm.tsx:132
- Draft comment:
Typo: It looks like "isTempoaray" might be a misspelling. Consider renaming it to "isTemporary". - Reason this comment was not posted:
Comment was on unchanged code.
9. frontend/libs/ui-modules/src/modules/segments/graphql/queries.ts:26
- Draft comment:
Typographical note: The query parameters in the SEGMENTS query are missing spaces. For example, it might be more readable as: query Segments($contentTypes: [String]!, $config: JSON, $ids: [String], $excludeIds: [String], $searchValue: String) { ... Consider adding spaces after commas and after colons where appropriate. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_RL8gXmBa1bY9EvPF
You can customize 
| export const getDefaultValues = ( | ||
| propertyType: string, | ||
| segment: any, | ||
| isTempoaray?: boolean, |
There was a problem hiding this comment.
Typo: The parameter name isTempoaray appears to be misspelled. Consider renaming it to isTemporary (or the intended spelling) for clarity.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
frontend/plugins/automations_ui/src/modules/app/components/editor/triggers/index.tsx (1)
29-29: Fix typo in prop name.There's a typo in the property name passed to the SegmentForm component.
- isTempoaray + isTemporaryfrontend/plugins/automations_ui/src/modules/app/components/editor/actions/If.tsx (1)
17-17: Fix typo in prop name.There's a typo in the property name passed to the SegmentForm component.
- isTempoaray + isTemporaryfrontend/libs/ui-modules/src/modules/segments/components/form/SegmentForm.tsx (3)
132-132: Fix typo in prop nameThere's a typo in the prop name:
isTempoarayshould beisTemporary.Apply this diff to fix the typo:
- isTempoaray, + isTemporary,
161-163: Redundant form configurationThe form has both
defaultValuesandvaluesset with the same function call, which is redundant sincevalueswill overridedefaultValues.Consider removing the redundant defaultValues:
const form = useForm<SegmentFormProps>({ resolver: zodResolver(segmentFormSchema), - defaultValues: getDefaultValues(contentType, segment || {}, isTempoaray), values: getDefaultValues(contentType, segment || {}, isTempoaray), });
319-320: Remove console.log from error handlerThe error handler in the form submission includes a console.log, which should be removed or replaced with proper error handling for production code.
Apply this diff to improve the error handling:
- (error) => console.log(error))} + (error) => toast({ + title: 'Validation Error', + description: 'Please check the form for errors.', + color: 'bg-red-500/50 text-red-500', + }))}frontend/libs/ui-modules/src/modules/segments/utils/index.tsx (3)
23-47: Field grouping logic with minor improvement opportunityThe
groupFieldsByTypefunction effectively organizes fields by group or value prefix, but could benefit from optional chaining.- const key = - value && value.includes('.') - ? value.substr(0, value.indexOf('.')) - : 'general'; + const key = + value?.includes('.') + ? value.substr(0, value.indexOf('.')) + : 'general';🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
98-133: Default values utility with typoThe
getDefaultValuesfunction correctly initializes form values, but there's a typo in the parameter name.- isTempoaray?: boolean, + isTemporary?: boolean,
135-160: Field configuration utility with defensive coding opportunitiesThe
getSelectedFieldConfigfunction correctly retrieves field configuration and operators, but could benefit from more defensive coding.- const { type, validation } = selectedField || {}; + const { type, validation } = selectedField;The
|| {}is redundant here since you already check forselectedFieldexistence before this line.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
frontend/core-ui/src/modules/app/components/SegmentsRoutes.tsx(1 hunks)frontend/core-ui/src/modules/segments/common/index.tsx(0 hunks)frontend/core-ui/src/modules/segments/components/SegmentDetail.tsx(1 hunks)frontend/core-ui/src/modules/segments/components/SegmentRecordTable.tsx(3 hunks)frontend/core-ui/src/modules/segments/components/SegmentsColumns.tsx(2 hunks)frontend/core-ui/src/modules/segments/components/SegmentsSidebar.tsx(1 hunks)frontend/core-ui/src/pages/segments/Detail.tsx(0 hunks)frontend/core-ui/src/pages/segments/SegmentsIndexPage.tsx(2 hunks)frontend/libs/ui-modules/src/modules/automations/common/Attributes.tsx(6 hunks)frontend/libs/ui-modules/src/modules/automations/common/PlaceHolderInput.tsx(0 hunks)frontend/libs/ui-modules/src/modules/automations/index.ts(1 hunks)frontend/libs/ui-modules/src/modules/segments/common/index.tsx(0 hunks)frontend/libs/ui-modules/src/modules/segments/components/QuerySelectInput.tsx(1 hunks)frontend/libs/ui-modules/src/modules/segments/components/SelectSegment.tsx(3 hunks)frontend/libs/ui-modules/src/modules/segments/components/form/SegmentForm.tsx(5 hunks)frontend/libs/ui-modules/src/modules/segments/components/form/SegmentGroup.tsx(2 hunks)frontend/libs/ui-modules/src/modules/segments/components/form/SegmentGroups.tsx(2 hunks)frontend/libs/ui-modules/src/modules/segments/components/form/SegmentProperty.tsx(3 hunks)frontend/libs/ui-modules/src/modules/segments/form/defaultValues.tsx(0 hunks)frontend/libs/ui-modules/src/modules/segments/graphql/index.ts(0 hunks)frontend/libs/ui-modules/src/modules/segments/graphql/mutations.ts(3 hunks)frontend/libs/ui-modules/src/modules/segments/graphql/queries.ts(3 hunks)frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx(2 hunks)frontend/libs/ui-modules/src/modules/segments/index.ts(1 hunks)frontend/libs/ui-modules/src/modules/segments/states/segmentFormSchema.tsx(1 hunks)frontend/libs/ui-modules/src/modules/segments/types/index.ts(1 hunks)frontend/libs/ui-modules/src/modules/segments/utils.ts(0 hunks)frontend/libs/ui-modules/src/modules/segments/utils/index.tsx(1 hunks)frontend/plugins/automations_ui/src/modules/app/components/editor/Header.tsx(0 hunks)frontend/plugins/automations_ui/src/modules/app/components/editor/actions/If.tsx(1 hunks)frontend/plugins/automations_ui/src/modules/app/components/editor/actions/ManageProperties.tsx(2 hunks)frontend/plugins/automations_ui/src/modules/app/components/editor/triggers/index.tsx(1 hunks)
💤 Files with no reviewable changes (8)
- frontend/libs/ui-modules/src/modules/automations/common/PlaceHolderInput.tsx
- frontend/core-ui/src/pages/segments/Detail.tsx
- frontend/plugins/automations_ui/src/modules/app/components/editor/Header.tsx
- frontend/libs/ui-modules/src/modules/segments/graphql/index.ts
- frontend/libs/ui-modules/src/modules/segments/form/defaultValues.tsx
- frontend/libs/ui-modules/src/modules/segments/common/index.tsx
- frontend/libs/ui-modules/src/modules/segments/utils.ts
- frontend/core-ui/src/modules/segments/common/index.tsx
🧰 Additional context used
🧬 Code Graph Analysis (8)
frontend/core-ui/src/modules/segments/components/SegmentDetail.tsx (1)
frontend/libs/ui-modules/src/modules/segments/types/index.ts (1)
ListQueryResponse(6-8)
frontend/libs/ui-modules/src/modules/segments/components/SelectSegment.tsx (1)
frontend/libs/ui-modules/src/modules/segments/graphql/queries.ts (1)
SEGMENTS(25-37)
frontend/core-ui/src/modules/app/components/SegmentsRoutes.tsx (2)
frontend/core-ui/src/pages/segments/SegmentsIndexPage.tsx (1)
SegmentsIndexPage(8-33)frontend/libs/erxes-ui/src/components/spinner.tsx (1)
Spinner(37-43)
frontend/plugins/automations_ui/src/modules/app/components/editor/actions/ManageProperties.tsx (1)
frontend/libs/ui-modules/src/modules/segments/utils/index.tsx (1)
groupFieldsByType(23-47)
frontend/libs/ui-modules/src/modules/segments/types/index.ts (1)
frontend/libs/ui-modules/src/modules/segments/states/segmentFormSchema.tsx (1)
segmentFormSchema(44-60)
frontend/core-ui/src/modules/segments/components/SegmentRecordTable.tsx (3)
frontend/libs/ui-modules/src/modules/segments/types/index.ts (1)
ListQueryResponse(6-8)frontend/libs/ui-modules/src/modules/segments/graphql/queries.ts (1)
SEGMENTS(25-37)frontend/core-ui/src/modules/segments/components/SegmentDetail.tsx (1)
SegmentDetail(13-68)
frontend/libs/ui-modules/src/modules/segments/components/form/SegmentGroups.tsx (1)
frontend/libs/ui-modules/src/modules/segments/components/form/SegmentGroup.tsx (1)
SegmentGroup(13-87)
frontend/libs/ui-modules/src/modules/segments/utils/index.tsx (2)
frontend/libs/ui-modules/src/modules/segments/states/segmentFormSchema.tsx (1)
segmentFormSchema(44-60)frontend/libs/ui-modules/src/modules/segments/types/index.ts (4)
IConditionsForPreview(141-144)SegmentFormProps(5-5)IField(62-80)IOperator(16-20)
🪛 Biome (1.9.4)
frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 80-80: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/libs/ui-modules/src/modules/segments/utils/index.tsx
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: core-ui
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (63)
frontend/libs/ui-modules/src/modules/automations/index.ts (1)
2-2: Expose PlaceHolderInput in the automations public API
This export correctly makesPlaceHolderInputavailable to consumers of the automations module, aligning with the refactoring goal of centralizing component exports.frontend/core-ui/src/modules/segments/components/SegmentsSidebar.tsx (1)
9-9: Good component renaming for clarity and consistency.Renaming from
ListSidebartoSegmentListSidebarmakes the component's purpose clearer and aligns with the broader refactoring pattern across the segments module.frontend/plugins/automations_ui/src/modules/app/components/editor/triggers/index.tsx (1)
4-4: Improved import structure.Updated import to use named export from the package root, which aligns with the broader refactoring of the ui-modules package and segment-related components.
frontend/plugins/automations_ui/src/modules/app/components/editor/actions/If.tsx (1)
2-2: Improved import organization.Streamlined imports by removing unused ones and consolidating imports from the ui-modules package. This makes the code cleaner and more maintainable.
frontend/libs/ui-modules/src/modules/segments/states/segmentFormSchema.tsx (1)
43-43: Improved code organization.Added spacing for better code readability. The removal of the
SegmentFormPropstype from this file and moving it to a dedicated types file (frontend/libs/ui-modules/src/modules/segments/types/index.ts) follows good practices for separation of concerns and centralized type management.frontend/libs/ui-modules/src/modules/segments/components/SelectSegment.tsx (3)
4-4: Import refactoring looks goodThe change from default import to named import (
SEGMENTS) aligns with the standardization of GraphQL queries as uppercase constants across the module.
39-39: Component name clarifiedRenaming from
SelectSegmentCommandtoSelectSegmentmakes the component name more concise while maintaining clarity about its purpose.
55-55: Query usage updated correctlyUsing the renamed
SEGMENTSconstant instead of the previousqueries.segmentsmaintains consistency with the new query import pattern.frontend/core-ui/src/modules/segments/components/SegmentsColumns.tsx (2)
24-24: Improved spacing with paddingAdding the
"pl-2"class to theRecordTableTree.Triggerprovides consistent left padding, improving the visual hierarchy of the tree structure.
74-74: Fixed width for actions columnSetting a fixed width of 40 pixels for the actions column ensures consistent layout across different screen sizes and prevents the column from taking unnecessary space.
frontend/core-ui/src/modules/app/components/SegmentsRoutes.tsx (4)
6-6: Better loading indicatorImporting the
Spinnercomponent from 'erxes-ui' provides a consistent loading experience across the application.
8-10: Updated import pathThe lazy-loaded component import has been correctly updated to reflect the renamed page component, maintaining consistency with the new file structure.
14-14: Enhanced loading experienceReplacing the simple fragment with a proper
Spinnercomponent improves the user experience during loading states.
17-17: Unified component for detail viewUsing the same
SegmentsIndexPagecomponent for both index and detail routes simplifies the codebase by consolidating related functionality into a single component.frontend/libs/ui-modules/src/modules/segments/types/index.ts (1)
2-3: Improved type safety with schema-based type inferenceUsing Zod's type inference to define
SegmentFormPropsfromsegmentFormSchemaensures that the type definition and validation schema remain in sync. This approach centralizes type definitions and leverages schema validation for better type safety.Also applies to: 5-5
frontend/libs/ui-modules/src/modules/segments/components/form/SegmentGroups.tsx (3)
3-4: Improved import paths for better maintainability.The imports have been updated to reflect the new component naming convention and type organization. The
SegmentGroupimport matches the renamed component, andSegmentFormPropsis now imported from a centralized types module instead of the schema file.
6-6: Component properly renamed fromSegmentstoSegmentGroups.This renaming provides better clarity about the component's purpose - it ma 8000 nages multiple segment groups rather than individual segments. This aligns with the component hierarchy where
SegmentGroupscontains multipleSegmentGroupcomponents.
75-75: Updated component reference to use the renamedSegmentGroup.The component reference has been updated to use the renamed
SegmentGroupcomponent (previously namedSegment), maintaining consistency with the new naming convention throughout the codebase.frontend/core-ui/src/modules/segments/components/SegmentDetail.tsx (3)
1-1: Simplified Apollo client imports.The import has been focused to include only the specific types needed from Apollo client, which is good practice.
5-5: Consolidated imports from ui-modules.The imports have been consolidated into a single line, importing multiple items from 'ui-modules'. This simplifies the import statements and makes the dependencies clearer.
13-13: Changed from default export to named export.The component has been changed from a default export to a named export, which improves consistency with the export style used in other components and makes imports more explicit.
frontend/plugins/automations_ui/src/modules/app/components/editor/actions/ManageProperties.tsx (2)
1-13: Reorganized and optimized imports.The import statements have been reorganized to group related imports together and remove redundant or unused imports. This improves code readability and maintainability.
182-182:✅ Verification successful
Updated to use new
groupFieldsByTypeutility.The component now uses the
groupFieldsByTypeutility instead of the deprecatedgroupByTypefunction. This aligns with the broader refactoring effort to reorganize segment-related utilities.
🏁 Script executed:
#!/bin/bash # Description: Verify the implementation of groupFieldsByType function # to ensure it maintains the same functionality as the previous groupByType function # Search for the implementation of groupFieldsByType rg -A 10 "export const groupFieldsByType" --type typescriptLength of output: 96
🏁 Script executed:
#!/bin/bash # Search for the new groupFieldsByType utility implementation rg -A 20 "export const groupFieldsByType" . # Fallback: locate any occurrence of groupFieldsByType rg -A 20 "groupFieldsByType" . # Locate the original groupByType implementation for comparison rg -A 20 "export const groupByType" .Length of output: 12937
Confirmed correct use of
groupFieldsByTypein place of deprecatedgroupByTypeVerified that the new utility is implemented in
frontend/libs/ui-modules/src/modules/segments/utils/index.tsxand is imported intoManageProperties.tsx, fully replacing the oldgroupByTypefunction with identical behavior.• File:
frontend/plugins/automations_ui/src/modules/app/components/editor/actions/ManageProperties.tsx
Line 182:const groups = groupFieldsByType(fields);
• Implementation:frontend/libs/ui-modules/src/modules/segments/utils/index.tsxexportsgroupFieldsByTypematching prior grouping logicNo further changes required.
frontend/libs/ui-modules/src/modules/segments/index.ts (2)
1-2: Improved GraphQL exports with explicit paths.The export statements for GraphQL operations have been refined to explicitly export from
./graphql/mutationsand./graphql/queriesinstead of the broader./graphql. This provides more clarity about the structure of the GraphQL operations and makes dependencies more explicit.
5-7: More granular and explicit exports for components and utilities.The export statements for components and utilities have been refined to be more explicit:
- Line 5: Explicitly exports from
./components/form/SegmentForm- Line 6: Explicitly exports the
FieldWithErrorcomponent- Line 7: Exports utilities from the new
./utilsmoduleThis provides better clarity about the available components and utilities and makes dependencies more explicit.
frontend/libs/ui-modules/src/modules/segments/components/form/SegmentGroup.tsx (3)
2-2: Imports are appropriately updated.The imports have been correctly restructured to match the new organization of the codebase, with
SegmentFormPropsnow imported from the types directory andSegmentPropertyfrom its renamed file.Also applies to: 4-5
13-18: Component renaming improves clarity.The component has been properly renamed from
SegmenttoSegmentGroupand converted to a named export, which better describes its functionality and aligns with the module's naming conventions.
57-57: Component reference correctly updated.The reference to the child component has been updated from
PropertytoSegmentProperty, ensuring consistency with the renamed component.frontend/core-ui/src/pages/segments/SegmentsIndexPage.tsx (3)
2-6: Imports consolidated and standardized.The imports have been properly reorganized, with UI components consolidated from
erxes-ui, component names updated to their new standardized names, and GraphQL query imported as a named constantSEGMENTS_GET_TYPESfromui-modules.
8-10: Component renamed with clear naming convention.The page component has been properly renamed from
SegmentstoSegmentsIndexPage, which better represents its purpose as the main index page for segments. The query usage is correctly updated to use the renamed constant.
29-30: JSX updated with renamed components.The JSX has been updated to use the renamed components
SegmentListSidebarandSegmentsRecordTable, maintaining consistency throughout the codebase.frontend/libs/ui-modules/src/modules/segments/components/form/SegmentProperty.tsx (4)
3-7: Imports appropriately reorganized and grouped.The imports have been properly reorganized to import from centralized locations, with hooks, types, utilities, and components clearly grouped. The addition of
QuerySelectInputsets up for the component replacement later in the file.Also applies to: 12-19
30-30: Utility function call updated.The call to
groupByTypehas been correctly updated togroupFieldsByType, reflecting its relocation to the new utilities module.
237-244: Enhanced selection component with multi-select support.The
SelectCommandcomponent has been replaced withQuerySelectInputwhich adds multi-select functionality through themultiprop. This improves usability by allowing users to select multiple items.
287-287: Component renamed for consistency.The component has been properly renamed from
PropertytoSegmentPropertyas a named export, which better describes its functionality and aligns with the module's naming conventions.frontend/core-ui/src/modules/segments/components/SegmentRecordTable.tsx (4)
3-6: Imports streamlined and standardized.The imports have been properly consolidated from
erxes-uiandui-modules, with component names updated to their standardized versions and direct import of theSEGMENTSquery.
37-37: Component renamed with clear naming convention.The component has been properly renamed from
ListtoSegmentsRecordTable, which better describes its functionality as a table displaying segment records.
40-42: GraphQL query usage updated.The query usage has been correctly updated to use the named export
SEGMENTSwith appropriate type annotation for the response.
62-63: Component reference updated in JSX.The JSX has been updated to use the renamed
SegmentDetailcomponent, maintaining consistency throughout the codebase.frontend/libs/ui-modules/src/modules/segments/graphql/mutations.ts (3)
3-25: Improved naming convention for GraphQL constantsThe renaming of template string constants from camelCase (
paramDefs,params) to uppercase (PARAMS_DEFS,PARAMS) follows best practices for constant naming conventions. This change improves code readability and maintains consistency with other GraphQL-related constants.
27-34: Better export pattern for GraphQL mutationsChanging from internally scoped constants to named exports with standardized uppercase naming (
SEGMENT_ADD) improves module accessibility and aligns with modern JavaScript best practices. This makes imports more explicit and improves maintainability.
36-43: Consistent export pattern applied to SEGMENT_EDITThe mutation constant has been standardized following the same pattern as SEGMENT_ADD, creating a consistent approach to GraphQL mutation exports throughout the codebase.
frontend/libs/ui-modules/src/modules/automations/common/Attributes.tsx (4)
3-7: Updated imports to use centralized module pathsThe import statements have been updated to use centralized module paths instead of relative paths, which improves maintainability as file locations change. The imports now correctly reference the renamed and relocated GraphQL query and hook.
50-50: Using the new hook for handling query select inputThe component now uses the new
useQuerySelectInputListhook, which provides better pagination support and consistent data fetching patterns across the application.
61-88: Improved UI structure with Collapsible componentThe UI has been restructured to use the Collapsible component, providing a more organized and user-friendly interface for the options display.
166-184: Consistent UI pattern with CollapsibleThe "Attributes" section has been updated to use the same Collapsible pattern as the other sections, creating a consistent user experience throughout the component.
frontend/libs/ui-modules/src/modules/segments/components/QuerySelectInput.tsx (4)
1-6: Good import organizationThe imports are well organized, starting with external libraries followed by React hooks and then local imports.
7-18: Well-structured component props interfaceThe Props type is clearly defined with appropriate types for all properties, making the component API clear and maintainable.
54-71: Well-implemented selection handlingThe selection handling logic is well-implemented, properly handling both single and multi-select modes with appropriate state updates.
93-97: Good pagination implementationThe pagination implementation using the Combobox.FetchMore component correctly handles loading more items when needed, with proper total count and current length tracking.
frontend/libs/ui-modules/src/modules/segments/components/form/SegmentForm.tsx (6)
20-30: Improved import organizationThe imports have been reorganized to use centralized module paths instead of relative paths, which improves maintainability. The renamed components and utilities are now correctly imported.
84-91: Updated component usage with SelectSegmentThe component now uses the renamed SelectSegment component, maintaining consistent naming throughout the codebase.
124-127: Updated component referencesComponent references have been updated from Segments to SegmentGroups and from Segment to SegmentGroup, aligning with the renamed components.
130-130: Export style changed to named exportThe component export has been changed from default export to named export, which is more consistent with modern JavaScript practices and improves explicitness in imports.
139-144: Updated GraphQL query and mutation referencesThe component now uses the renamed GraphQL constants, maintaining consistency with the changes in the GraphQL files.
250-253: Updated query reference and utility functionThe component now uses the renamed GraphQL constant and utility function, maintaining consistency with the changes in related files.
frontend/libs/ui-modules/src/modules/segments/graphql/queries.ts (3)
3-15: Good standardization of constant namingThe renaming of
segmentFieldstoSEGMENT_FIELDSfollows the common convention for constants. This is consistent with other constants in the file.One minor suggestion: Consider exporting
SEGMENT_FIELDSsince it's used by multiple queries and might be useful elsewhere in the codebase.
17-23: Well-structured GraphQL query exportsThe conversion from private/internal constants to explicitly exported uppercase constants improves the module's API and makes imports more explicit across the codebase.
Also applies to: 25-37, 39-43, 45-49, 51-56, 58-71, 73-89
61-61: Good reuse of SEGMENT_FIELDS constantUsing the
SEGMENT_FIELDSconstant in multiple places within theSEGMENT_DETAILquery improves maintainability. If the fields structure needs to change in the future, it only needs to be updated in one place.Also applies to: 63-63, 67-67
frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx (2)
1-10: Clean import organizationThe imports are well-organized, clearly separating Apollo Client, erxes-ui utilities, and local imports. The transition to named imports from the queries file aligns with the new export structure.
11-30: Update to named import usageThe function has been updated to use the renamed
PROPERTIES_WITH_FIELDSnamed export, maintaining consistency with the GraphQL query refactoring.frontend/libs/ui-modules/src/modules/segments/utils/index.tsx (2)
15-21: Clean string transformation utilityThe
startCasefunction effectively handles multiple string formats (camelCase, snake_case, kebab-case) and transforms them into a readable format.
51-58: Type-safe field path creatorThe
createFieldNameSafefunction properly handles path creation for react-hook-form with appropriate type safety.
| <Collapsible className="space-y-2"> | ||
| <Collapsible.TriggerButton className="px-4"> | ||
| <div className="flex items-center gap-2"> | ||
| <span>Options</span> | ||
| <Collapsible.TriggerIcon className="h-4 w-4" /> | ||
| </div> | ||
| </Collapsible.TriggerButton> | ||
| <Collapsible.Content className="px-4 m-0"> | ||
| <Command.Empty>Not found.</Command.Empty> | ||
| <Command.Group value="options"> | ||
| {selectOptions.map((option: any) => ( | ||
| <Command.Item | ||
| key={String(option.value)} | ||
| value={String(option.value)} | ||
| > | ||
| {option.label || '-'} | ||
| </Command.Item> | ||
| ))} | ||
| </Command.Group> | ||
| </Collapsible.Content> | ||
| </Collapsible> |
There was a problem hiding this comment.
Missing onSelect handler for static options
The Command.Item components in the static options don't have an onSelect handler, which means clicking on them won't do anything. This breaks functionality compared to the other Command.Item components that do have onSelect handlers.
Apply this diff to fix the issue:
<Command.Item
key={String(option.value)}
value={String(option.value)}
+ >
>📝 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.
| <Collapsible className="space-y-2"> | |
| <Collapsible.TriggerButton className="px-4"> | |
| <div className="flex items-center gap-2"> | |
| <span>Options</span> | |
| <Collapsible.TriggerIcon className="h-4 w-4" /> | |
| </div> | |
| </Collapsible.TriggerButton> | |
| <Collapsible.Content className="px-4 m-0"> | |
| <Command.Empty>Not found.</Command.Empty> | |
| <Command.Group value="options"> | |
| {selectOptions.map((option: any) => ( | |
| <Command.Item | |
| key={String(option.value)} | |
| value={String(option.value)} | |
| > | |
| {option.label || '-'} | |
| </Command.Item> | |
| ))} | |
| </Command.Group> | |
| </Collapsible.Content> | |
| </Collapsible> | |
| <Collapsible className="space-y-2"> | |
| <Collapsible.TriggerButton className="px-4"> | |
| <div className="flex items-center gap-2"> | |
| <span>Options</span> | |
| <Collapsible.TriggerIcon className="h-4 w-4" /> | |
| </div> | |
| </Collapsible.TriggerButton> | |
| <Collapsible.Content className="px-4 m-0"> | |
| <Command.Empty>Not found.</Command.Empty> | |
| <Command.Group value="options"> | |
| {selectOptions.map((option: any) => ( | |
| <Command.Item | |
| key={String(option.value)} | |
| value={String(option.value)} | |
| onSelect={onSelect} | |
| > | |
| {option.label || '-'} | |
| </Command.Item> | |
| ))} | |
| </Command.Group> | |
| </Collapsible.Content> | |
| </Collapsible> |
🤖 Prompt for AI Agents
In frontend/libs/ui-modules/src/modules/automations/common/Attributes.tsx around
lines 122 to 142, the Command.Item components inside the static options lack
onSelect handlers, causing clicks on these items to have no effect. Add an
onSelect handler to each Command.Item that triggers the appropriate selection
logic, matching the behavior of other Command.Item components with onSelect
handlers to restore expected functionality.
There was a problem hiding this comment.
@KhBaterdene I apologize for the confusion. Let me clarify my previous comment:
I noticed that in your renderOptions() function, the Command.Item components for static options don't have an onSelect handler, while similar components elsewhere in the file do have this handler (like in the attributes list and query-based options).
Without an onSelect handler, clicking on these option items won't trigger any action. Was this intentional, or should these items also call the onSelect function when clicked?
If they should be selectable, adding > to these Command.Items would make them behave consistently with the other selectable items in your component.
(_/)
(•.•)
(>❓<)
| const [search, setSearch] = useState(''); | ||
| const [value, setValue] = useState<string | string[]>(initialValue || ''); | ||
| const [open, setOpen] = useState(false); | ||
| const inputRef = useRef<HTMLInputElement>(null); | ||
| const [debouncedSearch] = useDebounce(search, 500); | ||
| const { | ||
| list = [], | ||
| totalCount = 0, | ||
| handleFetchMore, | ||
| } = useQuerySelectInputList(query, queryName, search); | ||
| const items = list.map((option: any) => ({ | ||
| label: option[labelField], | ||
| value: option[valueField], | ||
| })); | ||
| const selectedValue = items?.find( | ||
| (option: any) => option._id === value, | ||
| )?.value; | ||
|
|
There was a problem hiding this comment.
Inconsistent value selection logic
There's an inconsistency in how selected values are determined. Line 45-46 is comparing option._id === value, but options are mapped using valueField as the value property (lines 40-43).
Apply this diff to fix the issue:
const selectedValue = items?.find(
- (option: any) => option._id === value,
+ (option: any) => option.value === value,
)?.value;📝 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 [search, setSearch] = useState(''); | |
| const [value, setValue] = useState<string | string[]>(initialValue || ''); | |
| const [open, setOpen] = useState(false); | |
| const inputRef = useRef<HTMLInputElement>(null); | |
| const [debouncedSearch] = useDebounce(search, 500); | |
| const { | |
| list = [], | |
| totalCount = 0, | |
| handleFetchMore, | |
| } = useQuerySelectInputList(query, queryName, search); | |
| const items = list.map((option: any) => ({ | |
| label: option[labelField], | |
| value: option[valueField], | |
| })); | |
| const selectedValue = items?.find( | |
| (option: any) => option._id === value, | |
| )?.value; | |
| const [search, setSearch] = useState(''); | |
| const [value, setValue] = useState<string | string[]>(initialValue || ''); | |
| const [open, setOpen] = useState(false); | |
| const inputRef = useRef<HTMLInputElement>(null); | |
| const [debouncedSearch] = useDebounce(search, 500); | |
| const { | |
| list = [], | |
| totalCount = 0, | |
| handleFetchMore, | |
| } = useQuerySelectInputList(query, queryName, search); | |
| const items = list.map((option: any) => ({ | |
| label: option[labelField], | |
| value: option[valueField], | |
| })); | |
| const selectedValue = items?.find( | |
| (option: any) => option.value === value, | |
| )?.value; |
🤖 Prompt for AI Agents
In frontend/libs/ui-modules/src/modules/segments/components/QuerySelectInput.tsx
around lines 30 to 47, the selectedValue calculation incorrectly compares
option._id to value, but options use valueField as the value property. Update
the comparison to use option[valueField] === value instead of option._id ===
value to ensure consistent value selection logic.
frontend/libs/ui-modules/src/modules/segments/components/QuerySelectInput.tsx
Outdated
Show resolved
Hide resolved
| export const useQuerySelectInputList = ( | ||
| query: DocumentNode, | ||
| queryName: string, | ||
| searchValue: string, | ||
| skip?: boolean, | ||
| ) => { | ||
| const PER_PAGE = 30; | ||
| const { cursor } = useRecordTableCursor({ | ||
| sessionKey: 'property_cursor', | ||
| }); | ||
| const { data, loading, fetchMore } = useQuery(query, { | ||
| variables: { | ||
| limit: PER_PAGE, | ||
| cursor, | ||
| searchValue: searchValue ?? undefined, | ||
| }, | ||
| skip, | ||
| }); | ||
|
|
||
| const { list, totalCount, pageInfo } = (data || {})[queryName] || {}; | ||
|
|
||
| const selectedField = fields.find((field) => field.name === fieldName); | ||
| const handleFetchMore = ({ | ||
| direction, | ||
| }: { | ||
| direction: 'forward' | 'backward'; | ||
| onFetchMoreCompleted?: (fetchMoreResult: { | ||
| [queryName: string]: { | ||
| list: any[]; | ||
| }; | ||
| }) => void; | ||
| }) => { | ||
| if ( | ||
| (direction === 'forward' && pageInfo?.hasNextPage) || | ||
| (direction === 'backward' && pageInfo?.hasPreviousPage) | ||
| ) { | ||
| return fetchMore({ | ||
| variables: { | ||
| cursor: | ||
| direction === 'forward' | ||
| ? pageInfo?.endCursor | ||
| : pageInfo?.startCursor, | ||
| limit: PER_PAGE, | ||
| direction, | ||
| }, | ||
| updateQuery: (prev, { fetchMoreResult }) => { | ||
| if (!fetchMoreResult) return prev; | ||
|
|
||
| if (!selectedField) { | ||
| return undefined; | ||
| } | ||
| const { pageInfo: fetchMorePageInfo, list: fetchMoreList = [] } = | ||
| (fetchMoreResult || {})[queryName]; | ||
|
|
||
| const { type, validation } = selectedField || {}; | ||
| const { pageInfo: prevPageInfo, list: prevList = [] } = | ||
| (prev || {})[queryName] || {}; | ||
|
|
||
| const operatorType = (type || validation || '').toLowerCase() as | ||
| | 'string' | ||
| | 'boolean' | ||
| | 'number' | ||
| | 'date'; | ||
| // setCursor(prevPageInfo?.endCursor); | ||
|
|
||
| const operators = OPERATORS[operatorType] || DEFAULT_OPERATORS; | ||
| return Object.assign({}, prev, { | ||
| [queryName]: mergeCursorData({ | ||
| direction: EnumCursorDirection.FORWARD, | ||
| fetchMoreResult: { | ||
| pageInfo: fetchMorePageInfo, | ||
| list: fetchMoreList, | ||
| }, | ||
| prevResult: { | ||
| pageInfo: prevPageInfo, | ||
| list: prevList, | ||
| }, | ||
| }), | ||
| }); | ||
| }, | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| return { selectedField, operators }; | ||
| return { | ||
| list, | ||
| loading, | ||
| totalCount, | ||
| handleFetchMore, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
New pagination hook with optimization opportunities
The new useQuerySelectInputList hook provides a clean API for paginated data fetching with cursor-based pagination. However, there are a few opportunities for improvement:
- The code should use optional chaining for cleaner object access
- There's a commented out line of code that should be removed
- The direction type could be standardized with the enum used later
- const { list, totalCount, pageInfo } = (data || {})[queryName] || {};
+ const { list, totalCount, pageInfo } = data?.[queryName] ?? {};
- const { pageInfo: fetchMorePageInfo, list: fetchMoreList = [] } =
- (fetchMoreResult || {})[queryName];
+ const { pageInfo: fetchMorePageInfo, list: fetchMoreList = [] } =
+ fetchMoreResult?.[queryName] ?? {};
- const { pageInfo: prevPageInfo, list: prevList = [] } =
- (prev || {})[queryName] || {};
+ const { pageInfo: prevPageInfo, list: prevList = [] } =
+ prev?.[queryName] ?? {};
- // setCursor(prevPageInfo?.endCursor);Also, consider defining a type alias for the direction parameter to use the same enum throughout:
type CursorDirection = typeof EnumCursorDirection.FORWARD | typeof EnumCursorDirection.BACKWARD;🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 80-80: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🤖 Prompt for AI Agents
In frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx between lines
32 and 111, improve the useQuerySelectInputList hook by replacing manual object
access with optional chaining for cleaner and safer code, remove the commented
out line "setCursor(prevPageInfo?.endCursor);" as it is unused, and standardize
the direction parameter type by defining a type alias using the
EnumCursorDirection enum for both 'forward' and 'backward' directions to ensure
consistency throughout the hook.
| export const generateParamsSegmentPreviewCount = ( | ||
| form: UseFormReturn<z.infer<typeof segmentFormSchema>>, | ||
| selectedContentType: string, | ||
| ) => { | ||
| const conditions = form.watch('conditions'); | ||
| const conditionSegments = form.watch('conditionSegments'); | ||
| const conditionsConjunction = form.watch('conditionsConjunction'); | ||
|
|
||
| const conditionsForPreview: IConditionsForPreview[] = []; | ||
|
|
||
| if (conditions?.length) { | ||
| conditionsForPreview.push({ | ||
| type: 'subSegment', | ||
| subSegmentForPreview: { | ||
| key: Math.random().toString(), | ||
| contentType: selectedContentType || '', | ||
| conditionsConjunction, | ||
| conditions: conditions, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| if (conditionSegments?.length) { | ||
| conditionSegments.forEach((segment) => { | ||
| conditionsForPreview.push({ | ||
| type: 'subSegment', | ||
| subSegmentForPreview: { | ||
| key: Math.random().toString(), | ||
| ...segment, | ||
| conditions: segment.conditions || [], | ||
| }, | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| return conditionsForPreview; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using Math.random() for React keys
The generateParamsSegmentPreviewCount function works well for extracting conditions, but using Math.random().toString() for keys (lines 74 and 87) is not recommended for React.
- key: Math.random().toString(),
+ key: `segment_${Date.now()}_${Math.floor(Math.random() * 1000)}`,Consider using a more deterministic approach or a library like uuid for generating stable keys to prevent unnecessary re-renders.
📝 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 const generateParamsSegmentPreviewCount = ( | |
| form: UseFormReturn<z.infer<typeof segmentFormSchema>>, | |
| selectedContentType: string, | |
| ) => { | |
| const conditions = form.watch('conditions'); | |
| const conditionSegments = form.watch('conditionSegments'); | |
| const conditionsConjunction = form.watch('conditionsConjunction'); | |
| const conditionsForPreview: IConditionsForPreview[] = []; | |
| if (conditions?.length) { | |
| conditionsForPreview.push({ | |
| type: 'subSegment', | |
| subSegmentForPreview: { | |
| key: Math.random().toString(), | |
| contentType: selectedContentType || '', | |
| conditionsConjunction, | |
| conditions: conditions, | |
| }, | |
| }); | |
| } | |
| if (conditionSegments?.length) { | |
| conditionSegments.forEach((segment) => { | |
| conditionsForPreview.push({ | |
| type: 'subSegment', | |
| subSegmentForPreview: { | |
| key: Math.random().toString(), | |
| ...segment, | |
| conditions: segment.conditions || [], | |
| }, | |
| }); | |
| }); | |
| } | |
| return conditionsForPreview; | |
| }; | |
| export const generateParamsSegmentPreviewCount = ( | |
| form: UseFormReturn<z.infer<typeof segmentFormSchema>>, | |
| selectedContentType: string, | |
| ) => { | |
| const conditions = form.watch('conditions'); | |
| const conditionSegments = form.watch('conditionSegments'); | |
| const conditionsConjunction = form.watch('conditionsConjunction'); | |
| const conditionsForPreview: IConditionsForPreview[] = []; | |
| if (conditions?.length) { | |
| conditionsForPreview.push({ | |
| type: 'subSegment', | |
| subSegmentForPreview: { | |
| key: `segment_${Date.now()}_${Math.floor(Math.random() * 1000)}`, | |
| contentType: selectedContentType || '', | |
| conditionsConjunction, | |
| conditions: conditions, | |
| }, | |
| }); | |
| } | |
| if (conditionSegments?.length) { | |
| conditionSegments.forEach((segment) => { | |
| conditionsForPreview.push({ | |
| type: 'subSegment', | |
| subSegmentForPreview: { | |
| key: `segment_${Date.now()}_${Math.floor(Math.random() * 1000)}`, | |
| ...segment, | |
| conditions: segment.conditions || [], | |
| }, | |
| }); | |
| }); | |
| } | |
| return conditionsForPreview; | |
| }; |
🤖 Prompt for AI Agents
In frontend/libs/ui-modules/src/modules/segments/utils/index.tsx between lines
60 and 96, the function generateParamsSegmentPreviewCount uses
Math.random().toString() to generate keys for React elements, which is not
recommended as it causes unstable keys and unnecessary re-renders. Replace
Math.random() with a more stable and deterministic key generation method, such
as importing and using the uuid library to generate unique but stable keys for
each item in conditionsForPreview.
There was a problem hiding this comment.
Hey @Wlkr123 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| list = [], | ||
| totalCount = 0, | ||
| handleFetchMore, | ||
| } = useQuerySelectInputList(query, queryName, search); |
There was a problem hiding this comment.
suggestion (performance): Use debouncedSearch for query to avoid excessive network requests
Pass debouncedSearch to the hook instead of search to limit network requests to when the user stops typing.
| } = useQuerySelectInputList(query, queryName, search); | |
| } = useQuerySelectInputList(query, queryName, debouncedSearch); |
| conditionsForPreview.push({ | ||
| type: 'subSegment', | ||
| subSegmentForPreview: { | ||
| key: Math.random().toString(), |
There was a problem hiding this comment.
suggestion (bug_risk): Math.random-based keys can collide and aren't stable
Use a deterministic key, such as an incrementing ID or UUID, to ensure key stability and avoid potential collisions.
| } | ||
| setValue(updatedValue); | ||
| onSelect(updatedValue); | ||
| setOpen(false); |
There was a problem hiding this comment.
suggestion (bug_risk): Popover closes after every selection in multi mode
When multi is true, the popover should remain open after each selection to allow multiple choices.
Suggested implementation:
if (!multi) {
setOpen(false);
}-
Ensure that
multiis destructured from your component props, e.g.:const QuerySelectInput = ({ value, onSelect, multi = false, ... }) => { ... }
-
Verify that
multiis duly typed in your component’s TypeScript interface/props.
| direction, | ||
| }, | ||
| updateQuery: (prev, { fetchMoreResult }) => { | ||
| if (!fetchMoreResult) return prev; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!fetchMoreResult) return prev; | |
| if (!fetchMoreResult) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
|
|
||
| export const groupFieldsByType = (fields: any[]) => { | ||
| return fields.reduce((acc, field) => { | ||
| const value = field.value; |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
| const value = field.value; | |
| const {value} = field; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 22ff709 in 1 minute and 15 seconds. Click for details.
- Reviewed
40lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/libs/ui-modules/src/modules/segments/components/form/SegmentForm.tsx:24
- Draft comment:
Renamed import from getDefaultValues to getSegmentFormDefaultValues for clarity. Verify that temporary segment handling is maintained elsewhere. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is informative and asks the PR author to verify that a specific behavior is maintained. This violates the rule against asking the author to verify or ensure behavior. The first part of the comment is purely informative, which is also against the rules.
2. frontend/libs/ui-modules/src/modules/segments/components/form/SegmentForm.tsx:160
- Draft comment:
Removed 'isTempoaray' parameter when calling getSegmentFormDefaultValues. Confirm that the default values for temporary segments are handled as intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that the default values for temporary segments are handled as intended after the removal of a parameter. This falls under the rule of not asking the author to confirm their intention or ensure behavior is intended. Therefore, this comment should be removed.
3. frontend/libs/ui-modules/src/modules/segments/utils/index.tsx:98
- Draft comment:
Renamed getDefaultValues to getSegmentFormDefaultValues and removed the unused 'isTempoaray' parameter. Ensure that this simplification doesn't omit required logic for temporary segments. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%The comment is asking the author to ensure that the simplification doesn't omit required logic for temporary segments. This is a request for confirmation of intention, which violates the rules. However, the comment also provides specific information about the changes made, which could be useful for the author to consider. Despite this, the primary focus of the comment is on ensuring the behavior is intended, which is not allowed.
Workflow ID: wflow_vCjRexiDi2e8VyvF
You can customize
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
frontend/libs/ui-modules/src/modules/segments/utils/index.tsx (2)
25-25: Use object destructuring for cleaner codePrefer object destructuring when accessing and using properties for better readability.
- const value = field.value; + const { value } = field;
60-96:⚠️ Potential issueAvoid using Math.random() for React keys
Using
Math.random().toString()for React keys is not recommended as it can cause unnecessary re-renders and potentially lead to key collisions. Use a more deterministic approach for generating keys.- key: Math.random().toString(), + key: `segment_${selectedContentType}_${Date.now()}`,- key: Math.random().toString(), + key: `segment_${segment.contentType}_${Date.now()}_${conditionsForPreview.length}`,A better approach would be to use a library like
uuidif available, or use a combination of meaningful identifiers with timestamps to ensure uniqueness.
🧹 Nitpick comments (3)
frontend/libs/ui-modules/src/modules/segments/utils/index.tsx (3)
11-13: Consider organizing imports more consistentlyThe imports have an extra blank line between line 10 and 11. Consider grouping related imports together for better readability.
-import { Path } from 'react-hook-form'; -import { OPERATORS } from '../constants'; -import { DEFAULT_OPERATORS } from '../constants'; +import { Path } from 'react-hook-form'; +import { OPERATORS, DEFAULT_OPERATORS } from '../constants';
102-111: Remove unused destructured variableThe variable
getSubSegmentsis destructured but never used in the function. Consider removing it.const { subSegmentConditions = [], conditions = [], name, description, config, conditionsConjunction, subOf, - getSubSegments, } = segment;
148-148: Redundant null checkingThe null check at line 144 already ensures that
selectedFieldis defined, making the nullish coalescing in the destructuring redundant.- const { type, validation } = selectedField || {}; + const { type, validation } = selectedField;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/libs/ui-modules/src/modules/segments/components/form/SegmentForm.tsx(6 hunks)frontend/libs/ui-modules/src/modules/segments/utils/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/libs/ui-modules/src/modules/segments/components/form/SegmentForm.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/libs/ui-modules/src/modules/segments/utils/index.tsx (2)
frontend/libs/ui-modules/src/modules/segments/states/segmentFormSchema.tsx (1)
segmentFormSchema(44-60)frontend/libs/ui-modules/src/modules/segments/types/index.ts (4)
IConditionsForPreview(141-144)SegmentFormProps(5-5)IField(62-80)IOperator(16-20)
🪛 Biome (1.9.4)
frontend/libs/ui-modules/src/modules/segments/utils/index.tsx
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
frontend/libs/ui-modules/src/modules/segments/utils/index.tsx (4)
15-21: LGTM: Well-implemented utility functionThe
startCasefunction is well-implemented, handling different case formats (camelCase, snake_case, kebab-case) and converting them to a human-readable format.
49-58: LGTM: Well-typed utility functionThe
createFieldNameSafefunction is well-implemented and properly typed, correctly handling the filtering of invalid path parts.
98-132: LGTM: Good default values initializationThe function correctly initializes default form values for a segment, with appropriate fallbacks and prioritization logic.
134-159: LGTM: Well-structured field config retrievalThe
getSelectedFieldConfigfunction is well-structured with appropriate early returns and type handling.
| key = | ||
| value && value.includes('.') | ||
| ? value.substr(0, value.indexOf('.')) | ||
| : 'general'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use optional chaining to avoid potential null references
Use optional chaining to safely access the value property and check if it includes a dot.
- key =
- value && value.includes('.')
- ? value.substr(0, value.indexOf('.'))
- : 'general';
+ key =
+ value?.includes('.')
+ ? value.substring(0, value.indexOf('.'))
+ : 'general';Also note that substr is deprecated, so I've suggested using substring instead.
📝 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.
| key = | |
| value && value.includes('.') | |
| ? value.substr(0, value.indexOf('.')) | |
| : 'general'; | |
| key = | |
| value?.includes('.') | |
| ? value.substring(0, value.indexOf('.')) | |
| : 'general'; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🤖 Prompt for AI Agents
In frontend/libs/ui-modules/src/modules/segments/utils/index.tsx around lines 31
to 34, the code accesses `value` directly which may cause null reference errors
and uses the deprecated `substr` method. Update the code to use optional
chaining when checking if `value` includes a dot to prevent errors if `value` is
null or undefined. Replace `substr` with `substring` to avoid using deprecated
methods, ensuring the substring extraction logic remains the same.
| export const groupFieldsByType = (fields: any[]) => { | ||
| return fields.reduce((acc, field) => { | ||
| const value = field.value; | ||
| let key; | ||
|
|
||
| if (field.group) { | ||
| key = field.group; | ||
| } else { | ||
| key = | ||
| value && value.includes('.') | ||
| ? value.substr(0, value.indexOf('.')) | ||
| : 'general'; | ||
|
|
||
| key = startCase(key); | ||
| } | ||
|
|
||
| if (!acc[key]) { | ||
| acc[key] = []; | ||
| } | ||
|
|
||
| acc[key].push(field); | ||
|
|
||
| return acc; | ||
| }, {}); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper type definition for the fields parameter
The groupFieldsByType function uses any[] for the fields parameter, which reduces type safety. Consider using the IField[] type that's already imported.
-export const groupFieldsByType = (fields: any[]) => {
+export const groupFieldsByType = (fields: IField[]) => {Also, consider typing the return value for better type safety:
-export const groupFieldsByType = (fields: IField[]) => {
+export const groupFieldsByType = (fields: IField[]): Record<string, IField[]> => {📝 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 const groupFieldsByType = (fields: any[]) => { | |
| return fields.reduce((acc, field) => { | |
| const value = field.value; | |
| let key; | |
| if (field.group) { | |
| key = field.group; | |
| } else { | |
| key = | |
| value && value.includes('.') | |
| ? value.substr(0, value.indexOf('.')) | |
| : 'general'; | |
| key = startCase(key); | |
| } | |
| if (!acc[key]) { | |
| acc[key] = []; | |
| } | |
| acc[key].push(field); | |
| return acc; | |
| }, {}); | |
| }; | |
| export const groupFieldsByType = (fields: IField[]): Record<string, IField[]> => { | |
| return fields.reduce((acc, field) => { | |
| const value = field.value; | |
| let key; | |
| if (field.group) { | |
| key = field.group; | |
| } else { | |
| key = | |
| value && value.includes('.') | |
| ? value.substr(0, value.indexOf('.')) | |
| : 'general'; | |
| key = startCase(key); | |
| } | |
| if (!acc[key]) { | |
| acc[key] = []; | |
| } | |
| acc[key].push(field); | |
| return acc; | |
| }, {}); | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🤖 Prompt for AI Agents
In frontend/libs/ui-modules/src/modules/segments/utils/index.tsx around lines 23
to 47, the function groupFieldsByType uses any[] for the fields parameter, which
reduces type safety. Change the parameter type from any[] to IField[] to
leverage the existing imported interface. Additionally, explicitly type the
return value as a record or object with string keys and arrays of IField to
improve type safety and clarity.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 7ce8603 in 1 minute and 0 seconds. Click for details.
- Reviewed
221lines of code in4files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/libs/ui-modules/src/modules/segments/components/QuerySelectInput.tsx:36
- Draft comment:
Passing 'debouncedSearch' instead of 'search' to useQuerySelectInputList ensures queries are debounced, reducing unnecessary calls. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining the benefit of usingdebouncedSearchinstead ofsearch. It doesn't suggest any changes or ask for confirmation on the intention. According to the rules, purely informative comments should be removed.
2. frontend/libs/ui-modules/src/modules/segments/components/QuerySelectInput.tsx:70
- Draft comment:
Conditionally closing the popover only for single selections improves UX for multi-select cases. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it praises a change without providing any actionable feedback or suggestions. It doesn't align with the rules for useful comments in a pull request review.
3. frontend/libs/ui-modules/src/modules/segments/components/QuerySelectInput.tsx:80
- Draft comment:
Making Command.Input a controlled component (with value and onValueChange) ensures the search state is correctly managed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply acknowledges a change made in the code without offering any critique or guidance.
4. frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx:77
- Draft comment:
Adding braces for the updateQuery conditional improves readability without changing functionality. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is purely informative, suggesting a change for readability without indicating any potential issue or improvement in functionality. It doesn't ask for a specific action or provide a suggestion that aligns with the rules for good comments.
5. frontend/libs/ui-modules/src/modules/segments/utils/index.tsx:24
- Draft comment:
Destructuring the field and using optional chaining in groupFieldsByType increases safety against missing values. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it praises the use of destructuring and optional chaining without providing any actionable feedback or suggestions. It doesn't align with the rules for good comments, which should offer specific suggestions or ask for clarifications on potential issues.
6. frontend/plugins/automations_ui/src/modules/app/components/editor/actions/ManageProperties.tsx:196
- Draft comment:
Refactored the Property Type selection by removing FieldWithError and Form.Control wrappers; ensure that error display/styling is still handled as needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that error display/styling is still handled, which violates the rule against asking the author to ensure behavior is intended or tested. It does not provide a specific suggestion or ask for a specific test to be written.
7. frontend/plugins/automations_ui/src/modules/app/components/editor/actions/ManageProperties.tsx:79
- Draft comment:
The Rule component layout was refactored to use plain divs with Label instead of Form.Item; verify that this change maintains expected error handling and accessibility. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify that the change maintains expected error handling and accessibility, which violates the rule against asking the author to ensure behavior is intended or tested.
Workflow ID: wflow_R3CrJVVepltwnXF7
You can customize
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx (3)
87-87: Remove commented-out code.The commented-out line
// setCursor(prevPageInfo?.endCursor);should be removed as it's not being used.- // setCursor(prevPageInfo?.endCursor);
77-79: Use block braces for the if statement.Always use braces for if statements to improve code readability and prevent potential bugs when adding more statements later.
- if (!fetchMoreResult) { - return prev; - } + if (!fetchMoreResult) { + return prev; + }
51-51: 🛠️ Refactor suggestionUse optional chaining for cleaner object access.
Replace manual null/undefined checks with optional chaining and nullish coalescing for cleaner and more maintainable code.
- const { list, totalCount, pageInfo } = (data || {})[queryName] || {}; + const { list, totalCount, pageInfo } = data?.[queryName] ?? {}; - const { pageInfo: fetchMorePageInfo, list: fetchMoreList = [] } = - (fetchMoreResult || {})[queryName]; + const { pageInfo: fetchMorePageInfo, list: fetchMoreList = [] } = + fetchMoreResult?.[queryName] ?? {}; - const { pageInfo: prevPageInfo, list: prevList = [] } = - (prev || {})[queryName] || {}; + const { pageInfo: prevPageInfo, list: prevList = [] } = + prev?.[queryName] ?? {};Also applies to: 82-82, 85-85
🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/libs/ui-modules/src/modules/segments/components/QuerySelectInput.tsx(1 hunks)frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx(2 hunks)frontend/libs/ui-modules/src/modules/segments/utils/index.tsx(1 hunks)frontend/plugins/automations_ui/src/modules/app/components/editor/actions/ManageProperties.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/plugins/automations_ui/src/modules/app/components/editor/actions/ManageProperties.tsx
- frontend/libs/ui-modules/src/modules/segments/components/QuerySelectInput.tsx
- frontend/libs/ui-modules/src/modules/segments/utils/index.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx (4)
frontend/libs/ui-modules/src/modules/segments/types/index.ts (1)
FieldQueryResponse(82-85)frontend/libs/ui-modules/src/modules/segments/graphql/queries.ts (1)
PROPERTIES_WITH_FIELDS(51-56)frontend/libs/erxes-ui/src/modules/record-table/hooks/useRecordTableCursor.tsx (1)
useRecordTableCursor(4-21)frontend/libs/erxes-ui/src/modules/record-table/utils/cursorUtils.ts (1)
mergeCursorData(12-46)
🪛 Biome (1.9.4)
frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 82-82: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 85-85: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx (2)
15-15: Good refactoring to use named GraphQL query import.Using the named
PROPERTIES_WITH_FIELDSimport instead of a generic query import improves code clarity and maintainability.
32-113: Good implementation of the new paginated query hook.The
useQuerySelectInputListhook provides a clean implementation for paginated GraphQL list fetching with cursor-based pagination. This is a good replacement for the deprecateduseFieldSelectionListhook mentioned in the PR objectives, offering reusable functionality across the application.With the suggested refinements above, this will be a robust and maintainable solution.
🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 82-82: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 85-85: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| direction: 'forward' | 'backward'; | ||
| onFetchMoreCompleted?: (fetchMoreResult: { |
There was a problem hiding this comment.
Ensure consistency between direction parameter type and enum usage.
The direction parameter is typed as string literals 'forward' | 'backward', but EnumCursorDirection.FORWARD enum is used later. Additionally, the mergeCursorData call always uses FORWARD regardless of the actual direction parameter.
- }: {
- direction: 'forward' | 'backward';
+}: {
+ direction: typeof EnumCursorDirection.FORWARD | typeof EnumCursorDirection.BACKWARD;
// And then later in the code:
- [queryName]: mergeCursorData({
- direction: EnumCursorDirection.FORWARD,
+ [queryName]: mergeCursorData({
+ direction: direction === 'forward' ? EnumCursorDirection.FORWARD : EnumCursorDirection.BACKWARD,Consider defining a type alias to ensure consistency:
type CursorDirection = typeof EnumCursorDirection.FORWARD | typeof EnumCursorDirection.BACKWARD;Also applies to: 91-91
🤖 Prompt for AI Agents
In frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx around lines
56-57 and 91, the direction parameter is typed as string literals 'forward' |
'backward', but EnumCursorDirection enum values are used inconsistently, always
passing FORWARD to mergeCursorData. Define a type alias that matches the
EnumCursorDirection values and update the direction parameter type to use this
alias. Also, modify the mergeCursorData call to use the actual direction
parameter value instead of always FORWARD to ensure consistency.
| <Collapsible className="space-y-2"> | ||
| <Collapsible.TriggerButton className="px-4"> | ||
| <div className="flex items-center gap-2"> | ||
| <span>Options</span> | ||
| <Collapsible.TriggerIcon className="h-4 w-4" /> | ||
| </div> | ||
| </Collapsible.TriggerButton> | ||
| <Collapsible.Content className="px-4 m-0"> | ||
| <Command.Empty>Not found.</Command.Empty> | ||
| <Command.Group value="options"> | ||
| {selectOptions.map((option: any) => ( | ||
| <Command.Item | ||
| key={String(option.value)} | ||
| value={String(option.value)} | ||
| > | ||
| {option.label || '-'} | ||
| </Command.Item> | ||
| ))} | ||
| </Command.Group> | ||
| </Collapsible.Content> | ||
| </Collapsible> |
| @@ -1 +1,2 @@ | |||
| export * from './types'; | |||
| export * from './common/PlaceHolderInput'; | |||
There was a problem hiding this comment.
components folder luu oruulah common folder ustgah
| multi?: boolean; | ||
| }; | ||
|
|
||
| export const QuerySelectInput = ({ |
There was a problem hiding this comment.
hervee query gee commandtai ga tusad ni component uusgej gargaval zowhon popover neegdeh uyd querygee duudaad iluu optimal bolno
|
|
||
| useEffect(() => { | ||
| if (inputRef.current && focusOnMount) { | ||
| inputRef.current.focus(); |
There was a problem hiding this comment.
Command Input focusOnMount gedeg boolean utga propsoor awch baigaa
| defaultValues: getDefaultValues(contentType, segment || {}, isTempoaray), | ||
| values: getDefaultValues(contentType, segment || {}, isTempoaray), | ||
| defaultValues: getSegmentFormDefaultValues(contentType, segment || {}), | ||
| values: getSegmentFormDefaultValues(contentType, segment || {}), |
There was a problem hiding this comment.
?? eniig saijruulj bolohoor bol saijruulah
| const handleFetchMore = ({ | ||
| direction, | ||
| }: { | ||
| direction: 'forward' | 'backward'; |
| PlaceHolderInput, | ||
| IField as UIModuleField, | ||
| } from 'ui-modules'; | ||
| import { PROPERTY_OPERATOR } from '~/modules/constants'; |
| @@ -0,0 +1,158 @@ | |||
| import { UseFormReturn } from 'react-hook-form'; | |||
There was a problem hiding this comment.
filedaa todorhoi ner ogoh
| @@ -1,7 +1,7 @@ | |||
| import { useFormContext } from 'react-hook-form'; | |||
There was a problem hiding this comment.
yronhii nernees ali boloh tatgalzah
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed a5d9064 in 48 seconds. Click for details.
- Reviewed
148lines of code in7files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/libs/ui-modules/src/modules/segments/components/QuerySelectInput.tsx:43
- Draft comment:
selectedValue lookup uses 'option._id' but mapped items only have 'value'; consider replacing it with 'option.value' to ensure proper matching. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx:90
- Draft comment:
mergeCursorData call hardcodes EnumCursorDirection.FORWARD; use the provided 'direction' parameter to properly handle backward pagination. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_2mpEdrxzwTcZggKx
You can customize
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx (3)
51-51: Apply optional chaining for safer object access.The static analysis correctly identifies opportunities to use optional chaining for cleaner and safer code.
Apply this diff to use optional chaining:
- const { list, totalCount, pageInfo } = (data || {})[queryName] || {}; + const { list, totalCount, pageInfo } = data?.[queryName] ?? {}; - const { pageInfo: fetchMorePageInfo, list: fetchMoreList = [] } = - (fetchMoreResult || {})[queryName]; + const { pageInfo: fetchMorePageInfo, list: fetchMoreList = [] } = + fetchMoreResult?.[queryName] ?? {}; - const { pageInfo: prevPageInfo, list: prevList = [] } = - (prev || {})[queryName] || {}; + const { pageInfo: prevPageInfo, list: prevList = [] } = + prev?.[queryName] ?? {};Also applies to: 82-82, 85-85
🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
56-56: Fix type inconsistency and direction parameter usage.The direction parameter type doesn't match the enum usage, and the
mergeCursorDatacall always usesFORWARDregardless of the actual direction.Apply this diff to fix the type and direction usage:
- direction: EnumCursorDirection; + direction: 'forward' | 'backward'; - if ( - (direction === 'forward' && pageInfo?.hasNextPage) || - (direction === 'backward' && pageInfo?.hasPreviousPage) - ) { + if ( + (direction === EnumCursorDirection.FORWARD && pageInfo?.hasNextPage) || + (direction === EnumCursorDirection.BACKWARD && pageInfo?.hasPreviousPage) + ) { - direction: EnumCursorDirection.FORWARD, + direction: direction === 'forward' ? EnumCursorDirection.FORWARD : EnumCursorDirection.BACKWARD,Or alternatively, update the direction parameter to use the enum type:
- direction: 'forward' | 'backward'; + direction: EnumCursorDirection;Also applies to: 64-65, 91-91
87-87: Remove commented out code.The commented line should be removed as it's not being used.
Apply this diff:
- // setCursor(prevPageInfo?.endCursor);
🧹 Nitpick comments (4)
frontend/libs/ui-modules/src/modules/automations/components/Attributes.tsx (2)
16-25: Improve type safety with specific types.The component props use
anytypes which reduces type safety. Consider defining more specific interfaces.Apply this diff to improve type safety:
type Props = { contentType: string; - selectedField: any; - selectedOperator: any; + selectedField: { + selectOptions?: Array<{ label: string; value: string | number }>; + selectionConfig?: { + queryName: string; + labelField: string; + valueField?: string; + }; + }; + selectedOperator: { + name?: string; + value?: string; + }; onSelect: (value: string) => void; - customAttributions?: any[]; + customAttributions?: Array<{ name: string; label: string }>; - attrConfig?: any; + attrConfig?: Record<string, unknown>; onlySet?: boolean; trigger?: React.ReactNode; };
76-76: Inconsistent empty state handling.The
Command.Emptycomponent is placed outside ofCommand.Groupin the selection rendering but inside in the options rendering, which may cause inconsistent behavior.Move
Command.EmptyinsideCommand.Groupfor consistency:<Collapsible.Content className="px-4 m-0"> - <Command.Empty>Not found.</Command.Empty> <Command.Group value="queryOptions"> + <Command.Empty>Not found.</Command.Empty> <Combobox.Empty loading={loading} />Also applies to: 139-139
frontend/libs/ui-modules/src/modules/segments/utils/segmentFormUtils.tsx (2)
23-46: Improve string method usage and type safety.The function uses the deprecated
substr()method and has loose typing.Apply this diff to use modern string methods and improve types:
-export const groupFieldsByType = (fields: any[]) => { +export const groupFieldsByType = (fields: Array<{ value?: string; group?: string; [key: string]: any }>) => { return fields.reduce((acc: any, field) => { const { value } = field || {}; let key; if (field.group) { key = field.group; } else { key = value?.includes('.') - ? value.substr(0, value.indexOf('.')) + ? value.substring(0, value.indexOf('.')) : 'general'; key = startCase(key); }
73-73: Consider using crypto.randomUUID() for better uniqueness.Using
Math.random().toString()for generating keys may lead to collisions in edge cases.Apply this diff to use a more robust UUID generation:
+import { randomUUID } from 'crypto'; conditionsForPreview.push({ type: 'subSegment', subSegmentForPreview: { - key: Math.random().toString(), + key: randomUUID(), contentType: selectedContentType || '', conditionsConjunction, conditions: conditions, }, }); conditionsForPreview.push({ type: 'subSegment', subSegmentForPreview: { - key: Math.random().toString(), + key: randomUUID(), ...segment, conditions: segment.conditions || [], }, });Alternatively, if crypto is not available in the browser environment, consider using a more robust random ID generator like
nanoid.Also applies to: 86-86
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/libs/ui-modules/src/modules/automations/components/Attributes.tsx(1 hunks)frontend/libs/ui-modules/src/modules/automations/components/PlaceHolderInput.tsx(0 hunks)frontend/libs/ui-modules/src/modules/automations/index.ts(1 hunks)frontend/libs/ui-modules/src/modules/segments/components/QuerySelectInput.tsx(1 hunks)frontend/libs/ui-modules/src/modules/segments/components/form/SegmentForm.tsx(7 hunks)frontend/libs/ui-modules/src/modules/segments/components/form/SegmentProperty.tsx(3 hunks)frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx(2 hunks)frontend/libs/ui-modules/src/modules/segments/index.ts(1 hunks)frontend/libs/ui-modules/src/modules/segments/utils/segmentFormUtils.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/libs/ui-modules/src/modules/automations/components/PlaceHolderInput.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/libs/ui-modules/src/modules/automations/index.ts
- frontend/libs/ui-modules/src/modules/segments/components/form/SegmentProperty.tsx
- frontend/libs/ui-modules/src/modules/segments/index.ts
- frontend/libs/ui-modules/src/modules/segments/components/form/SegmentForm.tsx
- frontend/libs/ui-modules/src/modules/segments/components/QuerySelectInput.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 82-82: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 85-85: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
frontend/libs/ui-modules/src/modules/segments/hooks/index.tsx (2)
1-9: LGTM! Clean import structure with named exports.The refactoring to use named imports from GraphQL queries aligns well with the PR's objective of migrating to named exports for better clarity.
11-30: LGTM! Consistent usage of updated imports.The
getFieldsPropertiesfunction correctly uses the updatedPROPERTIES_WITH_FIELDSimport and maintains the same functionality.frontend/libs/ui-modules/src/modules/automations/components/Attributes.tsx (2)
111-119: LGTM! Proper GraphQL query usage.The component correctly uses the named export
FIELDS_COMBINED_BY_CONTENT_TYPEand handles the conditional configuration parameter appropriately.
155-201: LGTM! Well-structured component with proper UI patterns.The main component structure follows good React patterns with proper conditional rendering, collapsible sections, and appropriate use of UI components from the design system.
frontend/libs/ui-modules/src/modules/segments/utils/segmentFormUtils.tsx (4)
15-21: LGTM! Clean string transformation utility.The
startCasefunction correctly handles camelCase, snake_case, and kebab-case conversions with proper regex patterns.
50-57: LGTM! Type-safe field path construction.The
createFieldNameSafefunction provides good type safety with proper filtering of invalid path segments.
97-131: LGTM! Comprehensive default values extraction.The
getSegmentFormDefaultValuesfunction properly handles all the segment properties with appropriate fallbacks and conditional logic.
133-158: LGTM! Robust field configuration utility.The
getSelectedFieldConfigfunction includes proper null checks and fallback handling for operator selection based on field types.
| const query = gql` | ||
| query ${queryName}($searchValue: String,$direction: CURSOR_DIRECTION,$cursor: String,$limit:Int) { | ||
| ${queryName}(searchValue: $searchValue,direction:$direction,cursor:$cursor,limit:$limit) { | ||
| list{${labelField},${valueField}} | ||
| totalCount, | ||
| pageInfo { | ||
| hasNextPage | ||
| hasPreviousPage | ||
| startCursor | ||
| endCursor | ||
| } | ||
| } | ||
| } | ||
| `; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract GraphQL query construction to a constant or helper.
Constructing GraphQL queries as template strings within the component can lead to maintenance issues and potential security concerns.
Consider extracting the query construction to a dedicated function or constant:
+const createDynamicQuery = (queryName: string, labelField: string, valueField: string) => gql`
+ query ${queryName}($searchValue: String, $direction: CURSOR_DIRECTION, $cursor: String, $limit: Int) {
+ ${queryName}(searchValue: $searchValue, direction: $direction, cursor: $cursor, limit: $limit) {
+ list { ${labelField}, ${valueField} }
+ totalCount
+ pageInfo {
+ hasNextPage
+ hasPreviousPage
+ startCursor
+ endCursor
+ }
+ }
+ }
+`;
const renderSelection = ({ selectionConfig, onSelect }: SelectionProps) => {
const { queryName, labelField, valueField = '_id' } = selectionConfig || {};
- const query = gql`
- query ${queryName}($searchValue: String,$direction: CURSOR_DIRECTION,$cursor: String,$limit:Int) {
- ${queryName}(searchValue: $searchValue,direction:$direction,cursor:$cursor,limit:$limit) {
- list{${labelField},${valueField}}
- totalCount,
- pageInfo {
- hasNextPage
- hasPreviousPage
- startCursor
- endCursor
- }
- }
- }
- `;
+ const query = createDynamicQuery(queryName, labelField, valueField);🤖 Prompt for AI Agents
In frontend/libs/ui-modules/src/modules/automations/components/Attributes.tsx
around lines 38 to 51, the GraphQL query is constructed inline using template
strings, which can cause maintenance and security issues. Refactor by extracting
the query construction into a separate constant or helper function outside the
component. This helper should accept parameters like queryName, labelField, and
valueField and return the constructed query string, improving readability and
reusability.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 236735d in 2 minutes and 46 seconds. Click for details.
- Reviewed
2143lines of code in36files - Skipped
0files when reviewing. - Skipped posting
10draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/core-ui/src/modules/automations/graphql/automationQueries.ts:69
- Draft comment:
Ensure that the type for the $excludeIds variable is correct. It is declared as Boolean but might be expected to be an array of IDs in some contexts. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. frontend/core-ui/src/modules/logs/components/LogColumns.tsx:137
- Draft comment:
Consider using a more concise method (e.g., charAt(0)) for extracting the first character for the Avatar fallback instead of splitting the string. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. frontend/core-ui/src/modules/logs/hooks/useLogs.tsx:115
- Draft comment:
Consider adding error handling to the useQuery hook to catch and handle GraphQL errors, rather than only relying on the loading state. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. backend/core-api/src/modules/automations/graphql/schema/types.ts:43
- Draft comment:
Typographical consistency: the field definition 'tagIds:[String]' is missing a space after the colon. For better readability and consistency with other fields (e.g., 'updatedAt: Date'), consider changing it to 'tagIds: [String]'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While this is a real inconsistency, it's a very minor style issue. The code will work perfectly fine either way. The comment is technically correct and provides a concrete suggestion, but it's an extremely trivial change that doesn't impact functionality or maintainability in any meaningful way. The inconsistency could make the code slightly harder to read or maintain if different styles are mixed throughout the codebase. Some teams have strict style guidelines about spacing. While consistency is good, this is too minor to warrant a PR comment. Most teams would handle this kind of trivial style issue with automated formatting tools like Prettier rather than manual review comments. Delete this comment. While technically correct, it's too minor of a style issue to be worth a PR comment. This kind of formatting should be handled by automated tools.
5. frontend/core-ui/src/modules/automations/components/builder/AutomationBuilderHeader.tsx:230
- Draft comment:
Typo: The label text 'InActive' may be intended to be 'Inactive'. Please verify for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
6. frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterMenu.tsx:51
- Draft comment:
Typo detected: The class name "p-2m" on the component (line 51) appears to be a mistake. Please verify if this was intended or if it should be corrected (e.g., to "p-2"). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% Since this is a UI-related comment about styling and the rules explicitly state "Do NOT comment on any pure frontend UI or styling changes, only logic", this comment should be deleted. Even though the comment might be correct about the typo, we should assume the author knows what they want for UI styling. The typo could affect the layout in a way that impacts usability, and "p-2m" really does look like an obvious mistake that would be easy to fix. While true, the rules are very clear about not commenting on UI and styling changes. We must trust that the author has tested their UI changes and is satisfied with the appearance. Delete this comment as it violates the rule about not commenting on UI and styling changes.
7. frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterViews.tsx:4
- Draft comment:
Note: There appears to be a potential typographical/lexical inconsistency. The component is named 'LogActionsFilter' but imported from './LogActionFilter' (without the plural 's' in the file name). Please verify if this is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While there is a clear inconsistency, this is likely something that would be caught by the build if it was actually an issue. If the import works, then the file must exist with the correct name. This feels like a purely informational comment that doesn't require any actual code changes. The code will work fine either way. The inconsistent naming could potentially lead to confusion for future developers. It might be worth standardizing the naming convention. While consistency is good, this is a minor issue that doesn't affect functionality. The build system would fail if there was an actual problem with the import. Delete this comment. It's purely informational and doesn't require any code changes. If there was an actual import problem, the build would fail.
8. frontend/core-ui/src/modules/logs/components/filters/LogSourceFilter.tsx:21
- Draft comment:
Typographical fix: Consider renaming 'MongoDb' to 'MongoDB' for correct product name capitalization. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While technically correct about the official MongoDB capitalization, this is a UI label that's only shown to users. The difference between 'MongoDb' and 'MongoDB' is extremely minor and doesn't affect functionality. According to our rules, we should not comment on pure UI/styling changes, and capitalization falls into that category. The official branding of MongoDB might be important for consistency and professionalism. The fix is very simple and clear. While the fix is simple, our rules explicitly state not to comment on UI/styling changes and to assume the author made UI choices correctly. This is purely a visual/branding preference. Delete this comment as it relates to UI labeling which our rules explicitly say to ignore.
9. frontend/core-ui/src/modules/logs/components/filters/LogSourceFilter.tsx:26
- Draft comment:
Typographical fix: Consider renaming 'Graphql' to 'GraphQL' for correct term formatting. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While technically correct that 'GraphQL' is the proper capitalization, this is just a UI label text. According to the rules, we should not comment on pure frontend UI or styling changes - we should assume the author chose the UI text intentionally. This falls under UI presentation which we're told to ignore. The proper capitalization of technical terms could be considered part of correctness rather than just UI styling. Inconsistent technical term capitalization could confuse users. The rules explicitly state not to comment on UI/styling changes and to assume the author did UI changes correctly. This is clearly a UI label choice, even if it involves a technical term. Delete this comment as it relates to UI text formatting which we're explicitly told to ignore.
10. frontend/core-ui/src/modules/logs/components/filters/LogSourceFilter.tsx:36
- Draft comment:
Typographical fix: Consider renaming 'Api Request' to 'API Request' for consistent capitalization. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The rules explicitly state "Do NOT comment on any pure frontend UI or styling changes, only logic." This is purely about UI text formatting. While the suggestion might be valid, it's not within scope of what we should be commenting on. We should trust the author's UI decisions. The capitalization inconsistency could potentially cause confusion for users, and "API" is a widely accepted acronym that should be capitalized. While true, the rules are clear that UI and styling decisions should be left to the author. This is a UI labeling choice, not a logic issue. Delete this comment as it relates to UI text formatting, which is explicitly out of scope per the review rules.
Workflow ID: wflow_EfcWpij2Enf67yT7
You can customize
| }, | ||
| { | ||
| id: 'status', | ||
| accessorKey: 'status', | ||
| header: () => <RecordTable.InlineHead label="Status" />, | ||
| cell: ({ cell }) => { | ||
| const status = cell.getValue() as 'active' | 'draft'; | ||
| const [edit] = useMutation(AUTOMATION_EDIT); |
There was a problem hiding this comment.
Consider moving the useMutation hook out of the cell render function to prevent creating a new mutation instance on every render. This can improve performance in table cells.
| @@ -16,7 +17,7 @@ type QueryResponse = { | |||
| }; | |||
|
|
|||
| export const AutomationsRecordTable = () => { | |||
| const { data, loading, fetchMore } = useQuery<QueryResponse>( | |||
| const { data, loading, fetchMore, refetch } = useQuery<QueryResponse>( | |||
There was a problem hiding this comment.
The refetch variable is destructured from useQuery but never used. Consider removing it to clean up the code.
| const { data, loading, fetchMore, refetch } = useQuery<QueryResponse>( | |
| const { data, loading, fetchMore } = useQuery<QueryResponse>( |
| @@ -0,0 +1 @@ | |||
| export const generateListQueryParams = (params: any) => {}; | |||
There was a problem hiding this comment.
The function generateListQueryParams is currently a stub. Consider either implementing its logic or removing it if it remains unused.
| @@ -2,12 +2,14 @@ import contactResolvers from '@/contacts/graphql/resolvers/customResolvers'; | |||
| import productResolvers from 10000 '@/products/graphql/resolvers/customResolvers'; | |||
| import segmentResolvers from '@/segments/graphql/resolvers/customResolvers'; | |||
| import structureResolvers from '@/organization/structure/graphql/resolvers/customResolvers'; | |||
| // import logResolvers from '../../modules/logs/graphql/resolvers/customResolvers'; | |||
| import logResolvers from '../../modules/logs/graphql/resolvers/customResolvers'; | |||
| import automationsResolvers from '../../modules/automations/graphql/resolvers/customResolver'; | |||
There was a problem hiding this comment.
Typo alert: In the import for automationsResolvers, the module path is using customResolver (singular) while all other similar resolvers use customResolvers (plural). Please verify if this is an accidental typo.
| import automationsResolvers from '../../modules/automations/graphql/resolvers/customResolver'; | |
| import automationsResolvers from '../../modules/automations/graphql/resolvers/customResolvers'; |
| </div> | ||
| </RecordTableCellTrigger> | ||
| <RecordTableCellContent className="w-24 h-12 flex justify-center items-center space-x-2"> | ||
| <Label htmlFor="mode">InActive</Label> |
There was a problem hiding this comment.
Typo: The label text 'InActive' might be a typographical error. Consider changing it to 'Inactive' for standard capitalization.
| <Label htmlFor="mode">InActive</Label> | |
| <Label htmlFor="mode">Inactive</Label> |
| import { IconCheck } from '@tabler/icons-react'; | ||
| import { useSearchParams } from 'react-router'; | ||
|
|
||
| const getFieldLableOperator = ({ |
There was a problem hiding this comment.
Typographical error: The function name getFieldLableOperator appears to mistakenly use 'Lable' instead of 'Label'. Consider renaming it to getFieldLabelOperator for clarity.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
frontend/core-ui/src/modules/automations/components/AutomationsRecordTable.tsx (1)
2-2: Unused import detected.
IconChartPieis imported but not used in the component. Consider removing it to keep imports clean.-import { IconAffiliate, IconChartPie, IconSettings } from '@tabler/icons-react'; +import { IconAffiliate, IconSettings } from '@tabler/icons-react';frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterMenu.tsx (1)
11-68: Fix CSS class typo in custom field input.The component implementation is well-structured with proper state management and conditional rendering. However, there's a typo in the CSS class name.
Apply this diff to fix the typo:
- <Input - className="p-2m mt-2" - placeholder="Type a custom field name" + <Input + className="p-2 mt-2" + placeholder="Type a custom field name"frontend/core-ui/src/modules/logs/hooks/useLogs.tsx (1)
91-113: Remove commented code to improve maintainability.The large block of commented
useMultiQueryStatecode should be removed as it's no longer needed and adds unnecessary clutter to the codebase.Remove the entire commented block from lines 91-113.
backend/core-api/src/modules/automations/graphql/resolvers/customResolver/automations.ts (1)
5-15: Consider using underscore for unused resolver parameters.The empty object patterns
{}in the resolver functions are flagged by static analysis tools. While functionally correct, it's a best practice to use_for unused parameters.Apply this diff to follow GraphQL resolver conventions:
- async createdUser({ createdBy }: IAutomationDoc, {}, { models }: IContext) { + async createdUser({ createdBy }: IAutomationDoc, _args: any, { models }: IContext) { return await models.Users.findOne({ _id: createdBy }); }, - async updatedUser({ updatedBy }: IAutomationDoc, {}, { models }: IContext) { + async updatedUser({ updatedBy }: IAutomationDoc, _args: any, { models }: IContext) { return await models.Users.findOne({ _id: updatedBy }); }, - async tags({ tagIds }: IAutomationDoc, {}, { models }: IContext) { + async tags({ tagIds }: IAutomationDoc, _args: any, { models }: IContext) { return await models.Tags.find({ _id: { $in: tagIds } }); },🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 9-9: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 13-13: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
🪛 ESLint
[error] 5-5: Unexpected empty object pattern.
(no-empty-pattern)
[error] 9-9: Unexpected empty object pattern.
(no-empty-pattern)
[error] 13-13: Unexpected empty object pattern.
(no-empty-pattern)
frontend/core-ui/src/modules/automations/components/AutomationColumns.tsx (1)
74-74: Fix typo in label text.- <Label htmlFor="mode">InActive</Label> + <Label htmlFor="mode">Inactive</Label>backend/core-api/src/modules/logs/graphql/resolvers/queries.ts (1)
51-51: Use const instead of let for immutable variable.The
filtervariable is never reassigned after initialization.- let filter: any = {}; + const filter: any = {};🧰 Tools
🪛 ESLint
[error] 51-51: 'filter' is never reassigned. Use 'const' instead.
(prefer-const)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
backend/core-api/src/apollo/resolvers/queries.ts(2 hunks)backend/core-api/src/apollo/resolvers/resolvers.ts(1 hunks)backend/core-api/src/apollo/schema/schema.ts(3 hunks)backend/core-api/src/modules/automations/graphql/resolvers/customResolver/automations.ts(1 hunks)backend/core-api/src/modules/automations/graphql/resolvers/customResolver/index.ts(1 hunks)backend/core-api/src/modules/automations/graphql/schema/types.ts(1 hunks)backend/core-api/src/modules/logs/graphql/resolvers/customResolvers/log.ts(1 hunks)backend/core-api/src/modules/logs/graphql/resolvers/queries.ts(1 hunks)backend/core-api/src/modules/logs/graphql/schema/index.ts(3 hunks)backend/core-api/src/modules/tags/graphql/mutations.ts(1 hunks)frontend/core-ui/src/modules/automations/components/AutomationColumns.tsx(5 hunks)frontend/core-ui/src/modules/automations/components/AutomationRecordTableFilters.tsx(1 hunks)frontend/core-ui/src/modules/automations/components/AutomationsRecordTable.tsx(4 hunks)frontend/core-ui/src/modules/automations/components/builder/AutomationBuilder.tsx(1 hunks)frontend/core-ui/src/modules/automations/components/builder/AutomationBuilderHeader.tsx(4 hunks)frontend/core-ui/src/modules/automations/graphql/automationQueries.ts(1 hunks)frontend/core-ui/src/modules/logs/components/GraphqlLogDetailContent.tsx(1 hunks)frontend/core-ui/src/modules/logs/components/LogColumns.tsx(3 hunks)frontend/core-ui/src/modules/logs/components/LogsRecordTable.tsx(2 hunks)frontend/core-ui/src/modules/logs/components/LogsRecordTableFilter.tsx(0 hunks)frontend/core-ui/src/modules/logs/components/MongoLogDetailContent.tsx(1 hunks)frontend/core-ui/src/modules/logs/components/filters/LogActionFilter.tsx(1 hunks)frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterBarOperator.tsx(1 hunks)frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterBars.tsx(1 hunks)frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterDialogs.tsx(1 hunks)frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterMenu.tsx(1 hunks)frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterViews.tsx(1 hunks)frontend/core-ui/src/modules/logs/components/filters/LogSourceFilter.tsx(1 hunks)frontend/core-ui/src/modules/logs/components/filters/LogStatusFilter.tsx(1 hunks)frontend/core-ui/src/modules/logs/components/filters/LogUsersFilter.tsx(1 hunks)frontend/core-ui/src/modules/logs/components/filters/LogsRecordTableFilter.tsx(1 hunks)frontend/core-ui/src/modules/logs/constants/logFilter.ts(1 hunks)frontend/core-ui/src/modules/logs/graphql/logQueries.ts(1 hunks)frontend/core-ui/src/modules/logs/hooks/useLogs.tsx(1 hunks)frontend/core-ui/src/modules/logs/utils/logRecordTableUtils.ts(1 hunks)frontend/libs/erxes-ui/src/modules/record-table/utils/cursorUtils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/core-ui/src/modules/logs/components/LogsRecordTableFilter.tsx
✅ Files skipped from review due to trivial changes (9)
- frontend/core-ui/src/modules/logs/components/MongoLogDetailContent.tsx
- frontend/core-ui/src/modules/logs/components/GraphqlLogDetailContent.tsx
- frontend/core-ui/src/modules/automations/components/builder/AutomationBuilder.tsx
- backend/core-api/src/apollo/resolvers/queries.ts
- frontend/core-ui/src/modules/automations/graphql/automationQueries.ts
- frontend/core-ui/src/modules/logs/graphql/logQueries.ts
- backend/core-api/src/modules/automations/graphql/schema/types.ts
- frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterDialogs.tsx
- frontend/core-ui/src/modules/logs/constants/logFilter.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/core-ui/src/modules/automations/components/builder/AutomationBuilderHeader.tsx
🧰 Additional context used
🧬 Code Graph Analysis (11)
frontend/libs/erxes-ui/src/modules/record-table/utils/cursorUtils.ts (1)
frontend/libs/erxes-ui/src/modules/record-table/types/RecordTableCursorTypes.ts (1)
IRecordTableCursorPageInfo(1-6)
frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterViews.tsx (5)
frontend/libs/erxes-ui/src/modules/filter/components/Filter.tsx (1)
Filter(438-456)frontend/core-ui/src/modules/logs/components/filters/LogStatusFilter.tsx (1)
LogStatusFilter(8-45)frontend/core-ui/src/modules/logs/components/filters/LogSourceFilter.tsx (1)
LogSourceFilter(10-53)frontend/core-ui/src/modules/logs/components/filters/LogActionFilter.tsx (1)
LogActionsFilter(5-38)frontend/core-ui/src/modules/logs/components/filters/LogUsersFilter.tsx (1)
LogUsersFilter(4-37)
frontend/core-ui/src/modules/automations/components/AutomationRecordTableFilters.tsx (1)
frontend/libs/erxes-ui/src/modules/layout/components/Page.tsx (1)
PageSubHeader(17-31)
frontend/core-ui/src/modules/logs/components/filters/LogSourceFilter.tsx (2)
frontend/libs/erxes-ui/src/hooks/use-query-state.tsx (1)
useQueryState(6-49)frontend/libs/erxes-ui/src/components/combobox.tsx (1)
Combobox(203-211)
frontend/core-ui/src/modules/logs/components/filters/LogsRecordTableFilter.tsx (7)
frontend/libs/erxes-ui/src/hooks/use-query-state.tsx (1)
useMultiQueryState(57-98)frontend/libs/erxes-ui/src/modules/filter/components/Filter.tsx (1)
Filter(438-456)frontend/core-ui/src/modules/logs/constants/logFilter.ts (1)
LOGS_CURSOR_SESSION_KEY(81-81)frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterMenu.tsx (1)
LogRecordTableFilterMenu(11-68)frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterViews.tsx (1)
LogRecordTableFilterViews(7-30)frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterDialogs.tsx (1)
LogRecordTableFilterDialogs(3-11)frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterBars.tsx (1)
LogRecordTableFilterBars(36-214)
backend/core-api/src/modules/logs/graphql/schema/index.ts (1)
backend/plugins/frontline_api/src/modules/inbox/graphql/schemas/conversation.ts (1)
cursorParams(192-198)
frontend/core-ui/src/modules/logs/components/LogsRecordTable.tsx (2)
frontend/core-ui/src/modules/logs/hooks/useLogs.tsx (1)
useLogs(85-168)frontend/core-ui/src/modules/logs/constants/logFilter.ts (1)
LOGS_CURSOR_SESSION_KEY(81-81)
frontend/core-ui/src/modules/logs/components/filters/LogUsersFilter.tsx (3)
frontend/libs/erxes-ui/src/hooks/use-query-state.tsx (1)
useQueryState(6-49)frontend/libs/ui-modules/src/modules/team-members/types/TeamMembers.ts (1)
IMember(3-11)frontend/libs/ui-modules/src/modules/team-members/components/AssignMember.tsx (2)
AssignMemberList(66-103)AssignMemberItem(140-159)
frontend/core-ui/src/modules/automations/components/AutomationsRecordTable.tsx (2)
frontend/libs/erxes-ui/src/components/button.tsx (1)
Button(43-57)frontend/core-ui/src/modules/automations/components/AutomationRecordTableFilters.tsx (1)
AutomationRecordTableFilters(8-21)
frontend/core-ui/src/modules/logs/components/LogColumns.tsx (3)
frontend/libs/erxes-ui/src/modules/record-table/components/RecordTableCellInline.tsx (1)
RecordTableCellDisplay(76-95)frontend/libs/erxes-ui/src/components/avatar.tsx (1)
Avatar(72-75)frontend/libs/erxes-ui/src/utils/core.ts (1)
readFile(4-29)
backend/core-api/src/modules/logs/graphql/resolvers/queries.ts (4)
backend/erxes-api-shared/src/core-types/common.ts (2)
ICursorPaginateParams(18-24)ICursorPaginateResult(26-35)backend/erxes-api-shared/src/utils/mongo/cursor-util.ts (4)
getCursor(62-68)computeOperator(13-32)encodeCursor(42-60)getPaginationInfo(70-86)backend/core-api/src/connectionResolvers.ts (1)
IContext(182-186)backend/erxes-api-shared/src/core-types/modules/logs/logs.ts (1)
ILogDocument(28-30)
🪛 ESLint
frontend/core-ui/src/modules/logs/utils/logRecordTableUtils.ts
[error] 1-1: Unexpected empty arrow function.
(@typescript-eslint/no-empty-function)
backend/core-api/src/modules/logs/graphql/resolvers/customResolvers/log.ts
[error] 8-8: Unexpected empty object pattern.
(no-empty-pattern)
backend/core-api/src/modules/automations/graphql/resolvers/customResolver/automations.ts
[error] 5-5: Unexpected empty object pattern.
(no-empty-pattern)
[error] 9-9: Unexpected empty object pattern.
(no-empty-pattern)
[error] 13-13: Unexpected empty object pattern.
(no-empty-pattern)
backend/core-api/src/modules/logs/graphql/resolvers/queries.ts
[error] 51-51: 'filter' is never reassigned. Use 'const' instead.
(prefer-const)
🪛 Biome (1.9.4)
backend/core-api/src/modules/logs/graphql/resolvers/customResolvers/log.ts
[error] 8-8: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
backend/core-api/src/modules/automations/graphql/resolvers/customResolver/automations.ts
[error] 5-5: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 9-9: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 13-13: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterBars.tsx
[error] 170-170: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (34)
frontend/libs/erxes-ui/src/modules/record-table/utils/cursorUtils.ts (1)
23-28: Excellent safety improvement with optional chaining.The addition of optional chaining (
?.) operators prevents potential runtime errors whenfetchMoreResultorprevResultobjects arenullorundefined. This defensive programming approach aligns well with the interface definitions where properties likepageInfoandlistare optional.backend/core-api/src/modules/automations/graphql/resolvers/customResolver/index.ts (1)
1-4: Clean resolver export structure.The implementation correctly follows GraphQL resolver export conventions, importing the automation resolver and exporting it as part of an object structure. This integrates well with the broader automation resolver system.
backend/core-api/src/modules/tags/graphql/mutations.ts (1)
59-59: Proper integration of automation tagging support.The addition of
automation: models.Automationsto the modelMap correctly extends the tagging system to support automation entities. This follows the established pattern and enables consistent tagging behavior across different content types.backend/core-api/src/apollo/schema/schema.ts (1)
139-142: LGTM! Logs functionality properly re-enabled.The changes systematically re-integrate the logs GraphQL schema by importing the required types and queries and including them in the exported schema strings. This follows the same pattern as other modules and maintains consistency in the schema composition.
Also applies to: 181-181, 208-208
backend/core-api/src/apollo/resolvers/resolvers.ts (1)
5-6: LGTM! Resolver integration matches schema changes.The changes properly activate log resolvers and add automation resolvers, maintaining consistency with the schema integration in the previous file. The resolver export pattern follows the established convention.
Also applies to: 13-14
backend/core-api/src/modules/logs/graphql/resolvers/customResolvers/log.ts (1)
8-10: LGTM! Enhanced user resolution with full database lookup.The change from returning a stub object to performing an actual database query significantly improves the resolver functionality by providing full user documents instead of minimal references.
Regarding the static analysis hint about the empty object pattern
{}- this is a false positive. The empty object represents the GraphQLargsparameter which is unused in this resolver, following standard GraphQL resolver signature conventions.🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
🪛 ESLint
[error] 8-8: Unexpected empty object pattern.
(no-empty-pattern)
frontend/core-ui/src/modules/automations/components/AutomationRecordTableFilters.tsx (2)
15-17: Verify the conditional rendering logic for zero records.The current logic
totalCount ? '${totalCount} records found' : loading && <Skeleton.../>means that whentotalCountis0, it will show nothing (not even "0 records found") unlessloadingis true.Is this the intended behavior? You might want to consider:
- {totalCount - ? `${totalCount} records found` - : loading && <Skeleton className="w-20 h-4 inline-block mt-1.5" />} + {loading ? ( + <Skeleton className="w-20 h-4 inline-block mt-1.5" /> + ) : ( + `${totalCount} records found` + )}This would always show the record count when not loading, including "0 records found" when there are no results.
1-21: Well-structured component with clear purpose.The component follows React best practices with a focused responsibility, clear prop types, and appropriate use of UI library components. The styling and layout integration with
PageSubHeaderlooks consistent with the design system.frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterViews.tsx (2)
1-6: LGTM! Clean import structure.The imports are well-organized and all appear to be used in the component.
7-30: Excellent filter organization pattern.The component follows a clean, consistent pattern for organizing filter views. Each filter is properly wrapped in a
Filter.Viewwith appropriatefilterKeyprops. The use ofFilter.DateViewfor the date filter is appropriate and maintains consistency with the UI library's built-in components.frontend/core-ui/src/modules/automations/components/AutomationsRecordTable.tsx (3)
20-20: Good addition of refetch capability.Adding
refetchto the query destructuring enables potential data refresh functionality, which is a good practice for data tables.
64-67: Verify removal of breadcrumb navigation.The breadcrumb was changed from a navigational
Linkto a staticButton, removing the ability to navigate to settings. Please confirm this is intentional and aligns with the new UX design.The previous implementation provided navigation:
<Button variant="ghost" asChild> <Link to="/settings/automations"> <IconSettings /> Settings </Link> </Button>Current implementation is static:
<Button variant="ghost"> <IconAffiliate /> Automations </Button>
85-85: Excellent UX enhancement with filter component.The addition of
AutomationRecordTableFiltersprovides users with immediate feedback about the record count and loading state, improving the overall user experience.frontend/core-ui/src/modules/logs/components/filters/LogUsersFilter.tsx (3)
1-3: Clean import structure.All imports are properly used and organized logically.
4-19: Solid user selection logic implementation.The component correctly manages user selection state via URL query parameters and implements proper toggle logic. The null safety checks and array handling are well-implemented.
21-37: Excellent integration with existing components.The component properly integrates with
AssignMemberListandAssignMemberItemcomponents, and theCommandwrapper configuration is appropriate. TheshouldFilter={false}setting is correct sinceAssignMemberListhandles its own search/filter functionality.frontend/core-ui/src/modules/logs/components/filters/LogSourceFilter.tsx (3)
1-9: Well-chosen imports and setup.The icon imports are appropriate for each source type, and the component structure follows established patterns.
10-12: Proper state management setup.Using
useQueryStatefor the source filter enables URL persistence and proper state synchronization.
17-49: Excellent source options and selection logic.The predefined source options are comprehensive and well-labeled with appropriate icons. The toggle selection logic correctly clears the selection when the same item is clicked again, providing intuitive UX. The check icon display for selected items enhances usability.
frontend/core-ui/src/modules/logs/components/LogColumns.tsx (1)
34-40: Well-implemented user name generation with proper fallback logic.The function correctly prioritizes first/last name combination over email fallback, providing a consistent user display experience across the application.
frontend/core-ui/src/modules/logs/components/filters/LogsRecordTableFilter.tsx (1)
8-34: Excellent composition pattern implementation.The component effectively orchestrates all filter subcomponents and properly manages state through the
useMultiQueryStatehook. ThehasFilterslogic correctly determines filter state by checking for non-null values.frontend/core-ui/src/modules/logs/hooks/useLogs.tsx (2)
24-83: Well-implemented dynamic filter generation with comprehensive field support.The
generateFiltersfunction effectively handles both predefined filter fields and custom filters. The logic for prefixing custom filters withpayload.suggests proper backend integration for flexible filtering.
129-158: Robust pagination implementation with proper validation and data merging.The
handleFetchMorefunction includes proper validation throughvalidateFetchMoreand uses themergeCursorDatautility for safe data merging. This ensures reliable cursor-based pagination behavior.frontend/core-ui/src/modules/logs/components/LogsRecordTable.tsx (1)
10-17: Good refactoring to use custom hook and constants!The changes improve code maintainability by:
- Abstracting data fetching logic into the
useLogshook- Using the
LOGS_CURSOR_SESSION_KEYconstant instead of a hardcoded stringThis makes the component cleaner and more reusable.
Also applies to: 45-45
frontend/core-ui/src/modules/logs/components/filters/LogStatusFilter.tsx (1)
8-45: Well-implemented status filter component!The component follows React best practices with proper state management via URL query parameters and clean UI composition using erxes-ui components. The toggle logic and visual feedback are correctly implemented.
backend/core-api/src/modules/logs/graphql/schema/index.ts (2)
10-10: LGTM: Cursor field addition supports pagination.The addition of the
cursorfield to the Log type properly supports cursor-based pagination.
39-40: LGTM: Proper integration of cursor parameters.The integration of
cursorParamsintocommonQueryParamscorrectly enables cursor-based pagination for the logs queries.frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterBarOperator.tsx (2)
15-23: LGTM: Clean helper function implementation.The helper function properly finds and returns the operator label with appropriate fallback handling.
25-83: Well-designed operator selector component.The component correctly manages operator state per field using URL synchronization, provides appropriate type safety with the fieldName restriction, and implements clean UI interactions with proper visual feedback.
frontend/core-ui/src/modules/logs/components/filters/LogRecordTableFilterBars.tsx (3)
22-35: LGTM: Effective custom filter extraction logic.The helper function correctly identifies and extracts custom filter fields by excluding common filter names and operator fields.
52-64: LGTM: Proper user selection handler.The user selection logic correctly handles adding/removing users from the filter with appropriate array manipulation.
66-168: LGTM: Comprehensive filter bar implementation.The filter bars for standard fields (status, source, createdAt, action, userIds) are well-implemented with proper icons, operators, and UI interactions. The component correctly integrates with the filtering ecosystem.
frontend/core-ui/src/modules/automations/components/AutomationColumns.tsx (1)
25-31: LGTM! Clean user name formatting logic.The helper function properly handles cases where names might be undefined and provides a sensible fallback to email.
backend/core-api/src/modules/logs/graphql/resolvers/queries.ts (1)
86-236: Well-implemented cursor pagination logic!The implementation correctly handles:
- Forward and backward pagination
- Multi-field sorting with proper cursor conditions
- Edge cases for empty results
- Proper error handling with meaningful error messages
- Limit validation and capping at 50 records
| @@ -0,0 +1 @@ | |||
| export const generateListQueryParams = (params: any) => {}; | |||
There was a problem hiding this comment.
Complete the function implementation or remove if unused.
This empty function serves no purpose and will trigger ESLint errors. Additionally, the any parameter type lacks type safety.
Options to resolve:
- If the function is needed: Complete the implementation with proper typing:
-export const generateListQueryParams = (params: any) => {};
+/**
+ * Generates query parameters for list operations
+ * @param params - The parameters to convert to query format
+ * @returns Formatted query parameters
+ */
+export const generateListQueryParams = (params: Record<string, any>): Record<string, any> => {
+ // TODO: Implement parameter generation logic
+ return {};
+};- If the function is not needed: Remove the entire file to avoid dead code.
Would you like me to help implement the parameter generation logic based on the logs module requirements?
🧰 Tools
🪛 ESLint
[error] 1-1: Unexpected empty arrow function.
(@typescript-eslint/no-empty-function)
🤖 Prompt for AI Agents
In frontend/core-ui/src/modules/logs/utils/logRecordTableUtils.ts at line 1, the
function generateListQueryParams is currently empty and uses an untyped any
parameter, causing ESLint errors and lack of type safety. To fix this, either
fully implement the function with appropriate parameter types and logic based on
the logs module requirements or remove the function and the file entirely if it
is unused to avoid dead code.
| const { user = {}, userId } = cell?.row?.original || {}; | ||
| if (!userId) { | ||
| return ( | ||
| <RecordTableCellDisplay className="text-border"> | ||
| No User | ||
| </RecordTableCellDisplay> | ||
| ); | ||
| } | ||
| const { details } = user as IUser; | ||
| return ( | ||
| <RecordTableCellDisplay> | ||
| <AvatarPopover user={user} /> | ||
| <Avatar className="h-6 w-6 rounded-full"> | ||
| <Avatar.Image | ||
| src={readFile(details?.avatar)} | ||
| alt={details?.fullName || ''} | ||
| /> | ||
| <Avatar.Fallback className="rounded-lg text-black"> | ||
| {(details?.fullName || '').split('')[0]} | ||
| </Avatar.Fallback> | ||
| </Avatar> | ||
| {generateUserName(user)} | ||
| </RecordTableCellDisplay> | ||
| ); |
There was a problem hiding this comment.
Inconsistency between generateUserName function and Avatar fallback logic.
There's a discrepancy in how user names are handled:
generateUserNameusesuser.details?.firstNameanduser.details?.lastName- Avatar fallback uses
details?.fullName
This could lead to different display names in the avatar fallback versus the actual text.
Apply this diff to ensure consistency:
- <Avatar.Fallback className="rounded-lg text-black">
- {(details?.fullName || '').split('')[0]}
- </Avatar.Fallback>
+ <Avatar.Fallback className="rounded-lg text-black">
+ {generateUserName(user).charAt(0).toUpperCase()}
+ </Avatar.Fallback>Also consider adding type safety for the user object:
- const { details } = user as IUser;
+ const userDetails = (user as IUser)?.details || {};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/core-ui/src/modules/logs/components/LogColumns.tsx around lines 119
to 141, the avatar fallback uses details?.fullName while generateUserName uses
details?.firstName and details?.lastName, causing inconsistent display names. To
fix this, update the avatar fallback to use the same logic as generateUserName
by constructing the fallback from firstName and lastName or ensure both use
fullName consistently. Additionally, add type safety by properly typing the user
object as IUser to avoid runtime errors.
| key={value} | ||
| value={value} | ||
| className="cursor-pointer " | ||
| => setStatus(value === action ? '' : value)} |
There was a problem hiding this comment.
Update setter call to match the corrected function name.
Following the fix for the setter name, this line should also be updated.
Apply this diff:
- => setStatus(value === action ? '' : value)}
+ => setAction(value === action ? '' : value)}📝 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.
| onSelect={() => setStatus(value === action ? '' : value)} | |
| onSelect={() => setAction(value === action ? '' : value)} |
🤖 Prompt for AI Agents
In frontend/core-ui/src/modules/logs/components/filters/LogActionFilter.tsx at
line 28, the setter function name used in the onSelect callback is outdated.
Update the setter call to use the corrected function name to ensure state
updates correctly. Replace the old setter name with the new one in the onSelect
handler.
| import { Combobox, Command, useMultiQueryState, useQueryState } from 'erxes-ui'; | ||
|
|
||
| export const LogActionsFilter = () => { | ||
| const [action, setStatus] = useQueryState<string>('action'); |
There was a problem hiding this comment.
Fix naming inconsistency between state variable and setter.
The state variable is named action but the setter is named setStatus. This will cause issues when trying to update the action state.
Apply this diff to fix the naming:
- const [action, setStatus] = useQueryState<string>('action');
+ const [action, setAction] = useQueryState<string>('action');📝 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 [action, setStatus] = useQueryState<string>('action'); | |
| const [action, setAction] = useQueryState<string>('action'); |
🤖 Prompt for AI Agents
In frontend/core-ui/src/modules/logs/components/filters/LogActionFilter.tsx at
line 6, the state variable is named 'action' but its setter is incorrectly named
'setStatus'. Rename the setter to 'setAction' to match the state variable and
ensure proper state updates.
| const cursorParams = ` | ||
| limit: Int | ||
| cursor: String | ||
| direction: CURSOR_DIRECTION | ||
| cursorMode: CURSOR_MODE | ||
| `; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if orderBy field should be included in cursorParams.
The cursorParams fragment is missing the orderBy: JSON field that exists in a similar implementation in backend/plugins/frontline_api/src/modules/inbox/graphql/schemas/conversation.ts (lines 192-198). This might limit sorting capabilities for cursor-based pagination.
Please verify if this omission is intentional or if the orderBy field should be added for consistent sorting support:
🏁 Script executed:
#!/bin/bash
# Description: Search for other cursorParams definitions to check consistency
# Look for other cursorParams definitions in the codebase
rg -A 10 "cursorParams.*=" --type ts
# Search for orderBy usage in cursor pagination contexts
rg -A 5 -B 5 "orderBy.*JSON" --type tsLength of output: 3605
Include orderBy: JSON for consistent cursor-based pagination
The cursorParams block in backend/core-api/src/modules/logs/graphql/schema/index.ts currently omits the orderBy: JSON field, which is present in both
backend/plugins/frontline_api/src/modules/inbox/graphql/schemas/conversation.tsandbackend/erxes-api-shared/src/utils/apollo/constants.ts(GQL_CURSOR_PARAM_DEFS).
To align sorting capabilities across modules, update the definition or reuse the shared constant:
• File: backend/core-api/src/modules/logs/graphql/schema/index.ts
const cursorParams = `
limit: Int
cursor: String
direction: CURSOR_DIRECTION
cursorMode: CURSOR_MODE
+ orderBy: JSON
`;Alternatively, import and use GQL_CURSOR_PARAM_DEFS from backend/erxes-api-shared/src/utils/apollo/constants.ts to avoid duplication.
📝 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 cursorParams = ` | |
| limit: Int | |
| cursor: String | |
| direction: CURSOR_DIRECTION | |
| cursorMode: CURSOR_MODE | |
| `; | |
| const cursorParams = ` | |
| limit: Int | |
| cursor: String | |
| direction: CURSOR_DIRECTION | |
| cursorMode: CURSOR_MODE | |
| orderBy: JSON | |
| `; |
🤖 Prompt for AI Agents
In backend/core-api/src/modules/logs/graphql/schema/index.ts around lines 23 to
28, the cursorParams definition is missing the orderBy: JSON field, which is
necessary for consistent sorting in cursor-based pagination. To fix this, add
the orderBy: JSON field to the cursorParams string or alternatively import and
reuse the existing GQL_CURSOR_PARAM_DEFS constant from
backend/erxes-api-shared/src/utils/apollo/constants.ts to avoid duplication and
ensure consistency across modules.
| <Avatar.Fallback className="rounded-lg text-black"> | ||
| {(details?.fullName || '').split('')[0]} | ||
| </Avatar.Fallback> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the generateUserName helper for avatar fallback consistency.
The avatar fallback is using details?.fullName directly, but it should use the same name generated by the helper function to ensure consistency.
For the "Last Updated By" column:
- <Avatar.Fallback className="rounded-lg text-black">
- {(details?.fullName || '').split('')[0]}
- </Avatar.Fallback>
+ <Avatar.Fallback className="rounded-lg text-black">
+ {generateUserName(user).charAt(0).toUpperCase()}
+ </Avatar.Fallback>For the "Created By" column:
- <Avatar.Fallback className="rounded-lg text-black">
- {(details?.fullName || '').split('')[0]}
- </Avatar.Fallback>
+ <Avatar.Fallback className="rounded-lg text-black">
+ {generateUserName(user).charAt(0).toUpperCase()}
+ </Avatar.Fallback>Also applies to: 160-162
🤖 Prompt for AI Agents
In frontend/core-ui/src/modules/automations/components/AutomationColumns.tsx at
lines 137-139 and also lines 160-162, the Avatar.Fallback currently uses the
first character of details?.fullName directly. Replace this with the first
character of the string returned by the generateUserName helper function to
ensure consistent avatar fallback display. Update both instances accordingly by
calling generateUserName with the appropriate user details and extracting the
first character from its result.
| const status = cell.getValue() as 'active' | 'draft'; | ||
| const [edit] = useMutation(AUTOMATION_EDIT); | ||
| return ( | ||
| <div className="w-full flex justify-center"> | ||
| <Badge | ||
| variant={status === 'active' ? 'success' : 'secondary'} | ||
| className={cn('font-bold', { | ||
| 'text-accent-foreground': status !== 'active', | ||
| })} | ||
| > | ||
| {status} | ||
| </Badge> | ||
| </div> | ||
| <RecordTablePopover> | ||
| <RecordTableCellTrigger> | ||
| <div className="w-full flex justify-center"> | ||
| <Badge | ||
| variant={status === 'active' ? 'success' : 'secondary'} | ||
| className={cn('font-bold', { | ||
| 'text-accent-foreground': status !== 'active', | ||
| })} | ||
| > | ||
| {status} | ||
| </Badge> | ||
| </div> | ||
| </RecordTableCellTrigger> | ||
| <RecordTableCellContent className="w-24 h-12 flex justify-center items-center space-x-2"> | ||
| <Label htmlFor="mode">InActive</Label> | ||
| <Switch | ||
| id="mode" | ||
| => | ||
| edit({ | ||
| variables: { | ||
| id: cell.row.original._id, | ||
| status: open ? 'active' : 'draft', | ||
| }, | ||
| }) | ||
| } | ||
| checked={status === 'active'} | ||
| /> | ||
| </RecordTableCellContent> | ||
| </RecordTablePopover> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error handling for the mutation.
The status toggle mutation doesn't have error handling. Users should be notified if the status update fails.
- const [edit] = useMutation(AUTOMATION_EDIT);
+ const [edit] = useMutation(AUTOMATION_EDIT, {
+ onError: (error) => {
+ // Add appropriate error notification here
+ console.error('Failed to update automation status:', error);
+ },
+ });📝 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 status = cell.getValue() as 'active' | 'draft'; | |
| const [edit] = useMutation(AUTOMATION_EDIT); | |
| return ( | |
| <div className="w-full flex justify-center"> | |
| <Badge | |
| variant={status === 'active' ? 'success' : 'secondary'} | |
| className={cn('font-bold', { | |
| 'text-accent-foreground': status !== 'active', | |
| })} | |
| > | |
| {status} | |
| </Badge> | |
| </div> | |
| <RecordTablePopover> | |
| <RecordTableCellTrigger> | |
| <div className="w-full flex justify-center"> | |
| <Badge | |
| variant={status === 'active' ? 'success' : 'secondary'} | |
| className={cn('font-bold', { | |
| 'text-accent-foreground': status !== 'active', | |
| })} | |
| > | |
| {status} | |
| </Badge> | |
| </div> | |
| </RecordTableCellTrigger> | |
| <RecordTableCellContent className="w-24 h-12 flex justify-center items-center space-x-2"> | |
| <Label htmlFor="mode">InActive</Label> | |
| <Switch | |
| id="mode" | |
| onCheck 10000 edChange={(open) => | |
| edit({ | |
| variables: { | |
| id: cell.row.original._id, | |
| status: open ? 'active' : 'draft', | |
| }, | |
| }) | |
| } | |
| checked={status === 'active'} | |
| /> | |
| </RecordTableCellContent> | |
| </RecordTablePopover> | |
| const status = cell.getValue() as 'active' | 'draft'; | |
| - const [edit] = useMutation(AUTOMATION_EDIT); | |
| + const [edit] = useMutation(AUTOMATION_EDIT, { | |
| + onError: (error) => { | |
| + // Add appropriate error notification here | |
| + console.error('Failed to update automation status:', error); | |
| + }, | |
| + }); | |
| return ( | |
| <RecordTablePopover> | |
| <RecordTableCellTrigger> | |
| <div className="w-full flex justify-center"> | |
| <Badge | |
| variant={status === 'active' ? 'success' : 'secondary'} | |
| className={cn('font-bold', { | |
| 'text-accent-foreground': status !== 'active', | |
| })} | |
| > | |
| {status} | |
| </Badge> | |
| </div> | |
| </RecordTableCellTrigger> | |
| <RecordTableCellContent className="w-24 h-12 flex justify-center items-center space-x-2"> | |
| <Label htmlFor="mode">InActive</Label> | |
| <Switch | |
| id="mode" | |
| onCheckedChange={(open) => | |
| edit({ | |
| variables: { | |
| id: cell.row.original._id, | |
| status: open ? 'active' : 'draft', | |
| }, | |
| }) | |
| } | |
| checked={status === 'active'} | |
| /> | |
| </RecordTableCellContent> | |
| </RecordTablePopover> |
🤖 Prompt for AI Agents
In frontend/core-ui/src/modules/automations/components/AutomationColumns.tsx
around lines 57 to 88, the mutation call to update the status lacks error
handling. Wrap the mutation call in a try-catch block or use the onError
callback of the useMutation hook to catch errors. Display a user-friendly
notification or message when the mutation fails to inform users about the
failure.
| let filter = {}; | ||
| let filter: any = {}; | ||
|
|
||
| console.log({ d: params?.filters }); |
There was a problem hiding this comment.
Remove debugging console.log statements.
Console.log statements should be removed from production code.
- console.log({ d: params?.filters });
- console.log(JSON.stringify(filter));Also applies to: 66-66
🤖 Prompt for AI Agents
In backend/core-api/src/modules/logs/graphql/resolvers/queries.ts at lines 53
and 66, remove the console.log statements used for debugging to ensure no debug
output remains in the production code.
| function castValue(value: any, type: 'objectId' | 'date' | 'number') { | ||
| if (type === 'objectId') { | ||
| return new Types.ObjectId(value as string); | ||
| } | ||
|
|
||
| if (type === 'date') { | ||
| return new Date(value); | ||
| } | ||
|
|
||
| if (type === 'number') { | ||
| return Number(value); | ||
| } | ||
|
|
||
| return value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for type conversions.
The type conversion functions could fail with invalid input values.
function castValue(value: any, type: 'objectId' | 'date' | 'number') {
if (type === 'objectId') {
+ try {
return new Types.ObjectId(value as string);
+ } catch (error) {
+ throw new Error(`Invalid ObjectId value: ${value}`);
+ }
}
if (type === 'date') {
- return new Date(value);
+ const date = new Date(value);
+ if (isNaN(date.getTime())) {
+ throw new Error(`Invalid date value: ${value}`);
+ }
+ return date;
}
if (type === 'number') {
- return Number(value);
+ const num = Number(value);
+ if (isNaN(num)) {
+ throw new Error(`Invalid number value: ${value}`);
+ }
+ return num;
}
return value;
}📝 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.
| function castValue(value: any, type: 'objectId' | 'date' | 'number') { | |
| if (type === 'objectId') { | |
| return new Types.ObjectId(value as string); | |
| } | |
| if (type === 'date') { | |
| return new Date(value); | |
| } | |
| if (type === 'number') { | |
| return Number(value); | |
| } | |
| return value; | |
| } | |
| function castValue(value: any, type: 'objectId' | 'date' | 'number') { | |
| if (type === 'objectId') { | |
| try { | |
| return new Types.ObjectId(value as string); | |
| } catch (error) { | |
| throw new Error(`Invalid ObjectId value: ${value}`); | |
| } | |
| } | |
| if (type === 'date') { | |
| const date = new Date(value); | |
| if (isNaN(date.getTime())) { | |
| throw new Error(`Invalid date value: ${value}`); | |
| } | |
| return date; | |
| } | |
| if (type === 'number') { | |
| const num = Number(value); | |
| if (isNaN(num)) { | |
| throw new Error(`Invalid number value: ${value}`); | |
| } | |
| return num; | |
| } | |
| return value; | |
| } |
🤖 Prompt for AI Agents
In backend/core-api/src/modules/logs/graphql/resolvers/queries.ts around lines
70 to 84, the castValue function lacks error handling for invalid inputs during
type conversions. Wrap each type conversion (ObjectId, Date, Number) in
try-catch blocks to catch potential exceptions and handle them gracefully, such
as by throwing a descriptive error or returning a default/fallback value to
prevent runtime crashes.
| return JSON.parse(value); | ||
| } |
There was a problem hiding this comment.
Add error handling for JSON.parse.
The JSON.parse call could throw an error if the value is not valid JSON.
if (operator === 'exists') {
- return JSON.parse(value);
+ try {
+ return JSON.parse(value);
+ } catch (error) {
+ throw new Error(`Invalid boolean value for 'exists' operator: ${value}`);
+ }
}📝 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.
| return JSON.parse(value); | |
| } | |
| if (operator === 'exists') { | |
| try { | |
| return JSON.parse(value); | |
| } catch (error) { | |
| throw new Error(`Invalid boolean value for 'exists' operator: ${value}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In backend/core-api/src/modules/logs/graphql/resolvers/queries.ts around lines
44 to 45, the JSON.parse call can throw an error if the input value is not valid
JSON. Wrap the JSON.parse call in a try-catch block to handle potential parsing
errors gracefully. In the catch block, log the error or handle it appropriately
to prevent the application from crashing and provide meaningful feedback.
Summary by Sourcery
Refactor segments and automations modules by migrating to named GraphQL exports, introducing a paginated selection hook and QuerySelectInput component, restructuring and renaming form components, and enhancing UI with Collapsible panels.
New Features:
Enhancements:
Chores:
Important
Refactor segments and automations modules by migrating to named GraphQL exports, introducing a paginated selection hook and QuerySelectInput component, restructuring and renaming form components, and enhancing UI with Collapsible panels.
useQuerySelectInputListhook for paginated GraphQL list fetching.QuerySelectInputcomponent for combobox-based multi-/single-select UI.Collapsiblepanels for improved UX.SegmentForm,SegmentGroup,SegmentProperty).useFieldSelectionListwithuseQuerySelectInputListand update selection logic.groupFieldsByType,createFieldNameSafe,generateParamsSegmentPreviewCount) into a new utils module.console.logstatements and reorganize file hierarchy.This description was created by
for 236735d. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Chores