feat(filters): implement create filter functionality #2881
Conversation
- Add an API endpoint for creating filters - Include UI enhancements for filter creation dialog (Your filter creation process is so smooth, it's practically butter on a hot knife)
|
WalkthroughThe pull request introduces significant enhancements across various components in the workflows dashboard. Key updates include improved rendering and user interaction features in data tables, the addition of new filter creation functionalities, and modifications to UI definition components. The changes also involve updating TypeScript interfaces to accommodate new properties and refining error handling in service methods. Overall, the updates aim to enhance user experience and maintainability while ensuring that existing functionalities remain intact. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Outside diff range and nitpick comments (32)
packages/workflow-core/src/lib/plugins/common-plugin/workflow-token-plugin.ts (2)
Line range hint
38-47: Consider enhancing error handlingWhile the error logging is good, consider these improvements:
- Include the actual error object in the logger.error call
- Consider adding error type discrimination for more specific handling
Here's a suggested improvement:
} catch (error) { logger.error(`Rules Plugin Failed`, { name: this.name, ...uiDefinitionCreationArgs, + error: error instanceof Error ? error.message : String(error), + stack: error instanceof Error ? error.stack : undefined, }); return { callbackAction: this.errorAction, - error: error, + error: error instanceof Error ? error : new Error(String(error)), } as const; }
Line range hint
25-37: Consider adding input validationThe
invokemethod accepts workflow parameters but doesn't validate them before use.Consider adding validation:
async invoke(context: TContext) { const workflowRuntimeId = context.workflowRuntimeId as string; + + if (!workflowRuntimeId) { + throw new Error('WorkflowRuntimeId is required'); + } + + if (!this.uiDefinitionId) { + throw new Error('UiDefinitionId is required'); + } const uiDefinitionCreationArgs = { workflowRuntimeId: workflowRuntimeId, uiDefinitionId: this.uiDefinitionId, };apps/workflows-dashboard/src/pages/WorkflowDefinition/components/WorkflowDefinitionSummaryCard/WorkflowDefinitionSummaryCard.tsx (1)
19-50: Consider adding type safety for mapped itemsWhile the array-based approach is clean and maintainable, we could improve type safety by defining an interface for the mapped items.
Consider adding this type definition:
interface SummaryItem { label: string; value: string | JSX.Element; } const summaryItems: SummaryItem[] = [ { label: 'ID', value: workflowDefinition.id }, // ... rest of the items ];services/workflows-service/src/auth/workflow-token/workflow-token.service.ts (1)
46-53: Consider enhancing error handling granularityWhile using Promise.all for parallel execution is efficient, consider handling failures of individual promises separately to provide more specific fallback behaviors.
-const [uiDefinition, customer] = await Promise.all([ - this.uiDefinitionService.getByWorkflowDefinitionId( - workflowDefinitionId, - UiDefinitionContext.collection_flow, - [projectId], - ), - this.customerService.getByProjectId(projectId), -]); +let uiDefinition; +let customer; +try { + uiDefinition = await this.uiDefinitionService.getByWorkflowDefinitionId( + workflowDefinitionId, + UiDefinitionContext.collection_flow, + [projectId], + ); +} catch (error) { + uiDefinition = null; +} +try { + customer = await this.customerService.getByProjectId(projectId); +} catch (error) { + customer = { displayName: '' }; +}apps/workflows-dashboard/src/pages/WorkflowDefinition/components/EditorCard/EditorCard.tsx (4)
Line range hint
49-51: OptimizehasChangescomputation for performance and accuracyComparing stringified JSON objects to detect changes (lines 49-51) can be inefficient and may not handle non-serializable values correctly. Consider using a deep equality check function like
lodash.isEqualfor a more efficient and accurate comparison.
67-148: RefactorrenderDialogContentto separate componentsThe
renderDialogContentfunction (lines 67-148) is lengthy and handles multiple dialog modes within a single function. Extracting each dialog mode's content into separate components will improve modularity, readability, and maintainability.
86-113: Reduce code duplication between 'raw-edit' and 'no-code' dialog modesThe 'raw-edit' (lines 86-113) and 'no-code' (lines 115-143) cases in
renderDialogContenthave similar structures. Refactor the common elements into a shared component or function to eliminate duplication and enhance maintainability.Also applies to: 115-143
167-193: Simplify conditional className constructionsThe conditional
classNamestrings in theDialogTriggercomponents (lines 167-193) are complex and may affect readability. Utilize a utility library likeclassnamesorclsxto manage conditional classes more elegantly.apps/workflows-dashboard/src/pages/WorkflowDefinition/components/UIDefinitionEditor/UIDefinitionEditor.tsx (1)
35-154: Extract complex rendering logic into separate componentsThe
viewDialogContentprop contains deeply nested JSX with complex rendering logic (lines 35-154). Extracting this logic into separate components (e.g.,FormElements,Actions,Validations) will enhance readability and make the code easier to maintain and test.apps/workflows-dashboard/src/pages/WorkflowDefinition/WorkflowDefinition.tsx (1)
142-240: Extract plugin rendering into a separatePluginCardcomponentThe plugin rendering logic within the
viewDialogContentprop (lines 142-240) is extensive and nested. By extracting this logic into aPluginCardcomponent, you can improve code modularity, readability, and make it easier to manage and test individual plugin representations.apps/workflows-dashboard/src/pages/UIDefinitions/components/UIDefinitionsTable/components/CloneUIDefinitionButton/CloneUIDefinitionButton.tsx (3)
36-39: Avoid unnecessary type casting inhandleSubmitIn
handleSubmit, you're castingformDatatoanybefore assigning it tovalues. Instead, you can directly typeformDataas{ name: string }to avoid casting and improve type safety.Proposed change:
-const handleSubmit = useCallback( - async (formData: Record<string, any>) => { - const values: { name: string } = formData as any; +const handleSubmit = useCallback( + async (formData: { name: string }) => { + const values = formData; mutate({ uiDefinitionId, ...values }); }, [uiDefinitionId, mutate], );
43-47: Handle errors in mutation to provide user feedbackCurrently, if the
mutatefunction fails, the user is not informed. Consider adding error handling using theonErrorcallback ofuseCloneUIDefinitionMutationto display an error message to the user in case the cloning fails.
55-61: Ensure loading state is reflected in the formWhile you disable the form inputs with
disabled={isLoading}, ensure that the submit button reflects the loading state, such as showing a spinner or disabling it to prevent duplicate submissions.apps/workflows-dashboard/src/pages/Filters/Filters.tsx (2)
46-56: Add error handling tocreateFilterMutationCurrently, if the
createFilterMutationfails, the user does not receive any feedback. Consider adding anonErrorcallback to inform the user of any errors that occur during the filter creation process.
149-160: Handle loading state for workflow definitionsIf
workflowDefinitionsdata is still loading, the select input may be empty, causing confusion for the user. Consider adding anisLoadingcheck and disabling the select input or displaying a loading state until the data is available.Proposed change:
+const { data: workflowDefinitions, isLoading: isWorkflowDefinitionsLoading } = useWorkflowDefinitionsQuery(); ... <Select value={selectedWorkflow} onValueChange={setSelectedWorkflow} + disabled={isWorkflowDefinitionsLoading || createFilterMutation.isLoading} >packages/common/src/schemas/documents/schemas/entity-schema.ts (1)
96-96: Remove unnecessary commentThe comment
// No noseems redundant and may cause confusion. Consider removing it to improve code readability.Proposed change:
noownershipPercentage: Type.Optional(Type.Number()), // No no +noownershipPercentage: Type.Optional(Type.Number()),services/workflows-service/src/workflow/workflow.service.ts (1)
2112-2129: Refactor duplicated code for buildingcollectionFlowThe logic for building the
collectionFlowobject is duplicated in several places. To improve maintainability and reduce code duplication, consider extracting this logic into a separate helper function.Proposed change:
+function buildCollectionFlow(uiDefinition, customer) { + return buildCollectionFlowState({ + apiUrl: env.APP_API_URL, + steps: uiDefinition?.definition + ? getOrderedSteps( + (uiDefinition?.definition as Prisma.JsonObject)?.definition as Record< + string, + Record<string, unknown> + >, + { finalStates: [...WORKFLOW_FINAL_STATES] }, + ).map(stepName => ({ + stateName: stepName, + })) + : [], + additionalInformation: { + customerCompany: customer.displayName, + }, + }); +} ... // Replace duplicated code with the new helper function -const collectionFlow = buildCollectionFlowState({ /* ... */ }); +const collectionFlow = buildCollectionFlow(uiDefinition, customer);apps/workflows-dashboard/src/pages/Filters/helpers/deserialize-query-params.ts (1)
Line range hint
1-11: Consider documenting breaking changes and migration pathWhile these changes effectively support the new filter creation functionality, the addition of required
projectIdacross interfaces represents a breaking change that should be documented.Consider:
- Adding JSDoc comments explaining the
projectIdrequirement- Creating a migration guide for existing code
- Potentially providing a utility function to help migrate existing filter objects to the new format
apps/workflows-dashboard/src/domains/filters/query-keys.ts (1)
10-13: Consider namespacing the create query keyThe simple query key
['create']could potentially conflict with other create operations. Consider adding more context:create: () => ({ - queryKey: ['create'], + queryKey: ['filters', 'create'], queryFn: (dto: CreateFilterDto) => createFilter(dto), }),services/workflows-service/src/ui-definition/ui-definition.controller.ts (1)
38-43: Enhance name validation schemaConsider adding constraints to the name validation:
- Min/max length
- Format/pattern (if specific naming conventions are required)
- Trim whitespace
schema: Type.Object({ - name: Type.String(), + name: Type.String({ + minLength: 1, + maxLength: 100, + pattern: '^[a-zA-Z0-9-_\s]+$', + transform: ['trim'], + }), }),packages/workflow-core/src/lib/utils/definition-validator/states-validator.ts (1)
11-11: Add JSDoc documentation for the actions propertyPlease document the purpose and expected values of the actions property, especially the special NO_OP action.
+/** Actions to be performed during the transition. Special values: + * - NO_OP: Bypasses transition validation + */ actions?: string;services/workflows-service/src/ui-definition/ui-definition.service.ts (1)
96-101: Consider adding input validationThe method could benefit from validating the newName parameter before creating the UI definition.
async cloneUIDefinitionById(id: string, projectId: TProjectId, newName: string) { + if (!newName?.trim()) { + throw new Error('New name is required and cannot be empty'); + } const { createdAt, updatedAt,apps/workflows-dashboard/src/pages/Filters/components/FiltersTable/columns.tsx (2)
12-34: Enhance accessibility of the copy buttonWhile the copy functionality is well implemented, consider improving accessibility:
<button onClick={() => navigator.clipboard.writeText(info.getValue<string>())} className="text-gray-400 hover:text-gray-600" + aria-label="Copy ID to clipboard" + title="Copy ID to clipboard" >
51-58: Consider adding loading state for JSON viewThe JSON view button might benefit from a loading state when handling large JSON data:
<div className="flex flex-row items-center gap-3"> <JSONViewButton trigger={ <Eye className="h-5 w-5 cursor-pointer text-gray-600 transition-colors hover:text-blue-600" /> } json={JSON.stringify(info.getValue())} + loadingTrigger={ + <Eye className="h-5 w-5 animate-pulse text-gray-400" /> + } /> </div>apps/workflows-dashboard/src/pages/Filters/components/FiltersTable/FiltersTable.tsx (3)
24-24: Consider adding prop types validationThe component is memoized but lacks prop types validation. Consider adding PropTypes or making the Props interface more strict with readonly properties.
Line range hint
42-54: Improve error handling in sorting logicThe error handling for invalid sorting states could be more robust. Consider:
- Adding error boundary
- Providing user feedback for sorting errors
- Adding type guards for sorting state validation
onSortingChange: updater => { if (typeof updater === 'function') { const newSortingValue = updater(table.getState().sorting); table.setSorting(newSortingValue); } else { const sortingState = updater as SortingState; if (!sortingState[0]?.id) { - console.error(`Invalid sorting state: ${JSON.stringify(sortingState)}`); + const error = new Error(`Invalid sorting state: ${JSON.stringify(sortingState)}`); + console.error(error); + // Consider adding error reporting service + return; } onSort && onSort(sortingState[0]?.id, sortingState[0]?.desc ? 'desc' : 'asc'); } },
Line range hint
89-106: Optimize table rendering performanceThe table body rendering could be optimized:
- Consider virtualizing the table for large datasets
- Memoize row rendering functions
- Add pagination for better performance with large datasets
apps/workflows-dashboard/src/components/molecules/WorkflowsTable/columns.tsx (1)
82-91: Improve JSON viewer trigger accessibilityThe Eye icon button needs accessibility improvements:
- Add aria-label
- Add keyboard interaction support
- Consider adding tooltip
<Eye className="h-5 w-5 cursor-pointer text-gray-600 transition-colors hover:text-blue-600" + aria-label="View JSON context" + role="button" + tabIndex={0} + title="View JSON context" />apps/workflows-dashboard/src/pages/UIDefinitions/components/UIDefinitionsTable/columns.tsx (2)
17-19: Add error handling for clipboard operationsThe clipboard operation could fail in certain browsers or contexts. Add error handling to provide feedback to users.
-onClick={() => navigator.clipboard.writeText(info.getValue<string>())} +onClick={async () => { + try { + await navigator.clipboard.writeText(info.getValue<string>()); + // Optional: Add toast notification for success + } catch (error) { + console.error('Failed to copy to clipboard:', error); + // Optional: Add toast notification for failure + } +}}
99-103: Consider adding tooltip for truncated IDsLong workflow definition IDs might get truncated in the UI. Consider adding a tooltip to show the full ID.
-<span className="font-mono text-sm text-gray-600">{valueOrNA(info.getValue<string>())}</span> +<span + className="font-mono text-sm text-gray-600" + title={info.getValue<string>() || 'N/A'} +> + {valueOrNA(info.getValue<string>())} +</span>apps/workflows-dashboard/src/pages/WorkflowDefinitions/components/molecules/WorkflowDefinitionsTable/columns.tsx (1)
46-58: Consider extracting visibility badge to a reusable componentThe visibility badge implementation could be reused across the application where similar status indicators are needed.
// shared/components/StatusBadge.tsx type StatusVariant = 'success' | 'neutral'; interface StatusBadgeProps { variant: StatusVariant; label: string; } export const StatusBadge = ({ variant, label }: StatusBadgeProps) => { const styles = { success: 'bg-emerald-100 text-emerald-700 hover:bg-emerald-200', neutral: 'bg-slate-100 text-slate-700 hover:bg-slate-200', }; return ( <span className={`rounded-full px-3 py-1 text-xs font-medium transition-colors ${styles[variant]}`}> {label} </span> ); };services/workflows-service/src/workflow-defintion/workflow-definition.service.ts (1)
114-115: Add documentation for the new parametersThe addition of
projectIdandisPublicparameters should be documented to explain their purpose and impact.Add JSDoc comments to explain the parameters:
/** * Creates a copy of an existing workflow definition. * @param id - The ID of the workflow definition to copy * @param name - The name for the new copy * @param displayName - The display name for the new copy * @param projectId - The project ID to associate with the copy * @param options - Additional options for the copy * @param options.isPublic - Whether the copied definition should be public (defaults to false) * @returns The newly created workflow definition */ async copyDefinitionVersion(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (37)
apps/workflows-dashboard/src/components/molecules/WorkflowsTable/columns.tsx(2 hunks)apps/workflows-dashboard/src/domains/filters/filters.api.ts(2 hunks)apps/workflows-dashboard/src/domains/filters/filters.types.ts(2 hunks)apps/workflows-dashboard/src/domains/filters/query-keys.ts(1 hunks)apps/workflows-dashboard/src/domains/ui-definitions/ui-definitions.api.ts(1 hunks)apps/workflows-dashboard/src/domains/ui-definitions/ui-definitions.types.ts(2 hunks)apps/workflows-dashboard/src/pages/Filters/Filters.tsx(1 hunks)apps/workflows-dashboard/src/pages/Filters/components/FiltersTable/FiltersTable.tsx(1 hunks)apps/workflows-dashboard/src/pages/Filters/components/FiltersTable/columns.tsx(1 hunks)apps/workflows-dashboard/src/pages/Filters/helpers/deserialize-query-params.ts(1 hunks)apps/workflows-dashboard/src/pages/Filters/hooks/useCreateFilterMutation/index.ts(1 hunks)apps/workflows-dashboard/src/pages/Filters/hooks/useCreateFilterMutation/useCreateFilterMutation.ts(1 hunks)apps/workflows-dashboard/src/pages/Filters/types/filters-filter-values.ts(1 hunks)apps/workflows-dashboard/src/pages/Filters/types/filters-query-params.ts(1 hunks)apps/workflows-dashboard/src/pages/UIDefinitions/components/UIDefinitionsTable/columns.tsx(2 hunks)apps/workflows-dashboard/src/pages/UIDefinitions/components/UIDefinitionsTable/components/CloneUIDefinitionButton/CloneUIDefinitionButton.tsx(2 hunks)apps/workflows-dashboard/src/pages/UIDefinitions/helpers/deserialize-query-params.ts(1 hunks)apps/workflows-dashboard/src/pages/WorkflowDefinition/WorkflowDefinition.tsx(3 hunks)apps/workflows-dashboard/src/pages/WorkflowDefinition/components/EditorCard/EditorCard.tsx(2 hunks)apps/workflows-dashboard/src/pages/WorkflowDefinition/components/UIDefinitionEditor/UIDefinitionEditor.tsx(1 hunks)apps/workflows-dashboard/src/pages/WorkflowDefinition/components/WorkflowDefinitionSummaryCard/WorkflowDefinitionSummaryCard.tsx(2 hunks)apps/workflows-dashboard/src/pages/WorkflowDefinitions/components/molecules/WorkflowDefinitionsTable/WorkflowDefinitionsTable.tsx(1 hunks)apps/workflows-dashboard/src/pages/WorkflowDefinitions/components/molecules/WorkflowDefinitionsTable/columns.tsx(1 hunks)packages/common/src/schemas/documents/schemas/entity-schema.ts(1 hunks)packages/workflow-core/src/lib/plugins/common-plugin/types.ts(1 hunks)packages/workflow-core/src/lib/plugins/common-plugin/workflow-token-plugin.ts(1 hunks)packages/workflow-core/src/lib/utils/definition-validator/states-validator.ts(2 hunks)packages/workflow-core/src/lib/workflow-runner.ts(3 hunks)services/workflows-service/package.json(0 hunks)services/workflows-service/prisma/data-migrations(1 hunks)services/workflows-service/src/auth/workflow-token/workflow-token.service.ts(1 hunks)services/workflows-service/src/filter/filter.controller.external.ts(0 hunks)services/workflows-service/src/ui-definition/ui-definition.controller.ts(1 hunks)services/workflows-service/src/ui-definition/ui-definition.repository.ts(1 hunks)services/workflows-service/src/ui-definition/ui-definition.service.ts(1 hunks)services/workflows-service/src/workflow-defintion/workflow-definition.service.ts(1 hunks)services/workflows-service/src/workflow/workflow.service.ts(4 hunks)
💤 Files with no reviewable changes (2)
- services/workflows-service/package.json
- services/workflows-service/src/filter/filter.controller.external.ts
✅ Files skipped from review due to trivial changes (3)
- apps/workflows-dashboard/src/pages/Filters/hooks/useCreateFilterMutation/index.ts
- services/workflows-service/prisma/data-migrations
- apps/workflows-dashboard/src/pages/WorkflowDefinitions/components/molecules/WorkflowDefinitionsTable/WorkflowDefinitionsTable.tsx
🔇 Additional comments (14)
packages/workflow-core/src/lib/plugins/common-plugin/workflow-token-plugin.ts (1)
22-22: Verify the impact of empty persistResponseDestination
The persistResponseDestination property has been changed from 'metadata' to an empty string ''. This could affect how responses are persisted in the workflow.
Let's verify the usage of this property:
✅ Verification successful
Based on the search results, I can now provide a final response:
Empty persistResponseDestination is safe and intentional
The empty string initialization of persistResponseDestination in WorkflowTokenPlugin is correct. The codebase shows that:
- The property is optional in other plugins (ApiPlugin, vendor configurations)
- The workflow runner (
workflow-runner.ts) has proper null checks before using this value:if (apiPlugin.persistResponseDestination && responseBody) { // ... handle persistence }
- Other plugins in the codebase explicitly set specific destinations (e.g., 'entity.data', 'pluginsOutput.businessInformation') when persistence is needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how persistResponseDestination is used across the codebase
# Search for direct property usage
rg "persistResponseDestination" -A 3
# Search for metadata persistence patterns to understand the impact
ast-grep --pattern 'metadata = $_'
Length of output: 26268
apps/workflows-dashboard/src/pages/WorkflowDefinition/components/WorkflowDefinitionSummaryCard/WorkflowDefinitionSummaryCard.tsx (1)
5-5: LGTM! Enhanced visual hierarchy and readability
The styling improvements with gradients and shadows, along with the more descriptive header text, create a better visual hierarchy and user experience.
Also applies to: 15-17
services/workflows-service/src/auth/workflow-token/workflow-token.service.ts (1)
44-80: Verify the impact on workflow token creation
Let's ensure that the changes don't break existing workflow token creation flows.
✅ Verification successful
Based on the results, I can now generate the final response:
Error handling in workflow token creation is properly implemented
The changes to error handling in the workflow token service follow established patterns across the codebase:
- The error handling approach aligns with similar patterns found in other services
- The
createmethod is used correctly in integration tests, showing proper token creation flows - The fallback state provides safe defaults when UI definition or customer data is unavailable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of WorkflowTokenService.create
ast-grep --pattern 'workflowTokenService.create($$$)'
# Search for tests related to workflow token creation
rg -l "workflow.*token.*create" -g "*.test.ts" -g "*.spec.ts"
# Check for error handling patterns in similar services
rg "catch \(error\)" -g "*.service.ts" -A 5
Length of output: 12226
apps/workflows-dashboard/src/pages/Filters/types/filters-filter-values.ts (1)
2-2: Verify the impact of adding required projectId
Adding projectId as a required field is a breaking change that could affect existing code using this interface.
✅ Verification successful
No breaking changes detected with projectId addition
The addition of projectId as a required field in FiltersPageFilterValues is safe because:
- The interface is only used through
deserializeQueryParamswhich properly handles theprojectIdfield - The source
FiltersPageFilterQueryinterface already hasprojectIdas a required field - No direct instantiations of
FiltersPageFilterValueswere found in the codebase - All usages go through the filters provider and query parameter deserialization, which ensures
projectIdis always present
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usage of FiltersPageFilterValues to identify potential breaking changes
rg -t typescript "FiltersPageFilterValues" -B 2 -A 2
Length of output: 87
Script:
#!/bin/bash
# Search for FiltersPageFilterValues usage with ripgrep, without type restriction
rg "FiltersPageFilterValues" -B 2 -A 2
Length of output: 4790
Script:
#!/bin/bash
# Check the implementation of deserializeQueryParams to see how projectId is handled
cat apps/workflows-dashboard/src/pages/UIDefinitions/helpers/deserialize-query-params.ts apps/workflows-dashboard/src/pages/Filters/helpers/deserialize-query-params.ts
Length of output: 1025
Script:
#!/bin/bash
# Check the query params type definition and its usage
cat apps/workflows-dashboard/src/pages/Filters/types/filters-query-params.ts
# Also check if there are any direct usages of the interface without going through deserializeQueryParams
rg -g '!*.test.*' -g '!*.spec.*' "new.*FiltersPageFilterValues"
Length of output: 244
apps/workflows-dashboard/src/pages/Filters/types/filters-query-params.ts (1)
2-2: LGTM! Type definition is consistent
The required projectId field aligns with the type definition in FiltersPageFilterValues, maintaining consistency across interfaces.
apps/workflows-dashboard/src/domains/filters/filters.api.ts (1)
16-20: Verify error responses from the API
Let's verify how the API handles various error cases.
apps/workflows-dashboard/src/domains/filters/query-keys.ts (1)
1-2: Verify the createFilter API integration
Let's verify that the createFilter API endpoint is properly implemented and matches the expected DTO structure.
Also applies to: 10-13
✅ Verification successful
Let me gather more information about the DTO structure to complete the verification.
CreateFilter API integration is properly implemented
The implementation aligns perfectly with the expected structure:
- The API endpoint
/external/filtersis correctly used in the implementation - The
CreateFilterDtointerface matches the documented request structure with all required fields:- name (string)
- entity (string)
- query (with where and select objects)
- projectId (string)
- The API implementation properly sends the DTO to the endpoint and returns the response data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for createFilter API implementation
ast-grep --pattern 'export const createFilter = async (dto: CreateFilterDto) => {
$$$
}'
# Search for API endpoint definition
rg -A 5 "POST.*external/filters"
Length of output: 1381
Script:
#!/bin/bash
# Search for CreateFilterDto definition
ast-grep --pattern 'export interface CreateFilterDto {
$$$
}'
# Search for request body example in docs
rg -A 10 "example request body for creating a new filter"
Length of output: 2793
apps/workflows-dashboard/src/domains/ui-definitions/ui-definitions.types.ts (1)
16-16: Verify handling of new required 'name' property
Adding required name properties to existing interfaces IUIDefinition and CopyUIDefinitionDto could break existing code. Please ensure all usages have been updated.
Also applies to: 27-27
✅ Verification successful
Let me try a different search strategy to find usages of these interfaces.
Let me check one more thing to see how these interfaces are being used in the codebase.
Based on the evidence gathered, I can now provide the final response:
The new required 'name' property appears to be properly handled
The name property is correctly used in the codebase:
- In
copyUIDefinitionAPI call where it's properly passed in the request body - The mutation hook
useCloneUIDefinitionMutationhandles theCopyUIDefinitionDtowith the required name field - All UI Definition data fetching operations (
fetchUIDefinition,fetchUIDefinitions) expect the completeIUIDefinitioninterface including the name field
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for interface usages
ast-grep --pattern 'interface $_ extends IUIDefinition'
ast-grep --pattern 'const $_: IUIDefinition'
ast-grep --pattern 'function $_($$_: IUIDefinition)'
# Search for CopyUIDefinitionDto usages
ast-grep --pattern 'const $_: CopyUIDefinitionDto'
ast-grep --pattern 'function $_($$_: CopyUIDefinitionDto)'
Length of output: 5719
Script:
#!/bin/bash
# Search for files containing IUIDefinition or CopyUIDefinitionDto
rg -l "IUIDefinition|CopyUIDefinitionDto"
# Search for object literals that might be implementing these interfaces
ast-grep --pattern 'const $_ = {
$$$
} as IUIDefinition'
ast-grep --pattern 'const $_ = {
$$$
} as CopyUIDefinitionDto'
# Search for variable declarations and function parameters
rg ":\s*(IUIDefinition|CopyUIDefinitionDto)" -A 2
Length of output: 4502
Script:
#!/bin/bash
# Check the content of the files where these interfaces are used
cat apps/workflows-dashboard/src/domains/ui-definitions/ui-definitions.api.ts
cat apps/workflows-dashboard/src/pages/UIDefinitions/hooks/useCloneUIDefinitionMutation/useCloneUIDefinitionMutation.tsx
Length of output: 1903
apps/workflows-dashboard/src/domains/filters/filters.types.ts (1)
7-7: Verify handling of new required 'projectId' property
Adding a required projectId property to the existing IFilter interface could break existing code. Please ensure all usages have been updated.
✅ Verification successful
Let me gather more information about IFilter usages with a different approach.
Let me search for actual usages of the IFilter interface to understand how it's being used in the codebase.
Based on the search results, I can now generate the final response. The code shows that the IFilter interface is used in a filters feature where filters are created and managed through a REST API, and the projectId is a required field when creating new filters.
Breaking change concern is valid - update all filter creation code
The addition of the required projectId field to IFilter is indeed a breaking change that needs attention in the following locations:
apps/workflows-dashboard/src/domains/filters/filters.api.ts: ThecreateFilterAPI callservices/workflows-service/scripts/seed.ts: The filter creation in seed dataapps/workflows-dashboard/src/pages/Filters/Filters.tsx: The filter creation form
The code shows that new filters are already being created with projectId from the existing items: projectId: data?.items[0]?.projectId ?? '', indicating that the change has been properly handled in the UI.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for IFilter interface usages
ast-grep --pattern 'interface $_ extends IFilter'
ast-grep --pattern 'const $_: IFilter'
ast-grep --pattern 'function $_($$_: IFilter)'
Length of output: 2864
Script:
#!/bin/bash
# Search for IFilter usages with ripgrep for broader context
rg "IFilter" -A 3 -B 3
# Also search for filter-related components and hooks that might use IFilter
rg "useFilter|FilterComponent|filter\(" -A 3 -B 3
Length of output: 68135
Script:
#!/bin/bash
# Search for actual implementations and usages of IFilter
rg -A 5 "implements IFilter|extends IFilter|: IFilter \{|: IFilter;|as IFilter"
# Also search for filter-related API calls or hooks that might use IFilter
rg -A 5 "useFilter|getFilter|createFilter|updateFilter"
Length of output: 66802
services/workflows-service/src/ui-definition/ui-definition.controller.ts (1)
50-54: LGTM! Clean implementation following NestJS practices
The implementation correctly handles the new name parameter and follows controller best practices.
services/workflows-service/src/ui-definition/ui-definition.repository.ts (1)
54-54: LGTM! Improved query construction
The change correctly optimizes the OR condition by spreading args?.where only once, eliminating potential duplicate conditions.
services/workflows-service/src/ui-definition/ui-definition.service.ts (1)
86-86: LGTM! Method signature enhancement
The addition of the newName parameter aligns well with the filter creation functionality requirements.
services/workflows-service/src/workflow-defintion/workflow-definition.service.ts (1)
114-115: Verify the impact of hardcoded isPublic value
Setting isPublic: false by default for copied definitions might not be the desired behavior in all cases. Consider making this configurable.
-async copyDefinitionVersion(
- id: string,
- name: string,
- displayName: string,
- projectId: TProjectId,
-) {
+async copyDefinitionVersion(
+ id: string,
+ name: string,
+ displayName: string,
+ projectId: TProjectId,
+ options: { isPublic?: boolean } = {},
+) {
// ...
const createArgs = replaceNullsWithUndefined(
merge(restArgs, {
name,
displayName,
projectId,
- isPublic: false,
+ isPublic: options.isPublic ?? false,
version: 1,
}),
) as Prisma.WorkflowDefinitionCreateArgs['data'];✅ Verification successful
Based on the search results and schema analysis, I can now provide a final response:
Setting isPublic to false is the correct default behavior
The codebase shows a clear pattern where isPublic is used to distinguish between shared/template workflow definitions (isPublic: true) and project-specific workflow definitions (isPublic: false). The schema enforces this through a constraint: CHECK (("isPublic" = true AND "projectId" IS NULL) OR ("isPublic" = false")).
When copying a workflow definition:
- If it's meant to be a template, it should be explicitly set as public through a separate process
- Project-specific copies should remain private by default for security
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any direct references to isPublic in workflow definitions
rg "isPublic.*workflow.*definition" -A 5 -B 5
# Check for any UI components that might be affected by this change
ast-grep --pattern 'isPublic.*=.*true'
Length of output: 88
Script:
#!/bin/bash
# Let's try a broader search for isPublic in workflow related files
rg "isPublic" -A 5 -B 5 services/workflows-service/
# Check the schema definition to understand the isPublic field
fd "schema.prisma" --exec cat {}
# Check for any existing copy operations
rg "copy.*workflow.*definition" -i
Length of output: 70957
packages/workflow-core/src/lib/workflow-runner.ts (1)
470-472: LGTM! Useful addition for debugging and tracking.
The NO_OP action provides a clean way to track workflow state transitions without side effects.
...orkflowDefinition/components/WorkflowDefinitionSummaryCard/WorkflowDefinitionSummaryCard.tsx
Outdated
Show resolved
Hide resolved
...orkflowDefinition/components/WorkflowDefinitionSummaryCard/WorkflowDefinitionSummaryCard.tsx
Show resolved
Hide resolved
services/workflows-service/src/auth/workflow-token/workflow-token.service.ts
Show resolved
Hide resolved
services/workflows-service/src/auth/workflow-token/workflow-token.service.ts
Show resolved
Hide resolved
apps/workflows-dashboard/src/pages/WorkflowDefinition/components/EditorCard/EditorCard.tsx
Show resolved
Hide resolved
apps/workflows-dashboard/src/components/molecules/WorkflowsTable/columns.tsx
Outdated
Show resolved
Hide resolved
apps/workflows-dashboard/src/pages/UIDefinitions/components/UIDefinitionsTable/columns.tsx
Outdated
Show resolved
Hide resolved
...oard/src/pages/WorkflowDefinitions/components/molecules/WorkflowDefinitionsTable/columns.tsx
Outdated
Show resolved
Hide resolved
…itor - Ensure components correctly return when encountering undefined types - Clean up nested element rendering logic for clarity (if your nested elements were any deeper, they'd need their own zip code)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
apps/workflows-dashboard/src/pages/WorkflowDefinition/components/UIDefinitionEditor/UIDefinitionEditor.tsx (2)
34-34: MakeenableViewModeprop configurableThe
enableViewModeprop is hardcoded totrue. Consider making it configurable by passing it as a prop from the parent component to allow for more flexibility in different usage contexts.- enableViewMode={true} + enableViewMode={props.enableViewMode}
96-106: Add error boundary for recursive renderingThe deep recursive rendering of nested elements could potentially cause runtime errors. Consider adding an error boundary to gracefully handle such cases.
class ElementErrorBoundary extends React.Component<{ children: React.ReactNode }> { state = { hasError: false }; static getDerivedStateFromError() { return { hasError: true }; } render() { if (this.state.hasError) { return <div className="text-red-500">Error rendering element</div>; } return this.props.children; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/workflows-dashboard/src/pages/WorkflowDefinition/components/UIDefinitionEditor/UIDefinitionEditor.tsx(1 hunks)
🔇 Additional comments (1)
apps/workflows-dashboard/src/pages/WorkflowDefinition/components/UIDefinitionEditor/UIDefinitionEditor.tsx (1)
Line range hint 1-197: Implementation approved with suggested improvements
The changes successfully implement a view mode for UI definitions with a good separation between view and edit modes. While the core functionality is solid, consider implementing the suggested improvements for better maintainability, type safety, and error handling.
...-dashboard/src/pages/WorkflowDefinition/components/UIDefinitionEditor/UIDefinitionEditor.tsx
Show resolved
Hide resolved
...-dashboard/src/pages/WorkflowDefinition/components/UIDefinitionEditor/UIDefinitionEditor.tsx
Show resolved
Hide resolved
...-dashboard/src/pages/WorkflowDefinition/components/UIDefinitionEditor/UIDefinitionEditor.tsx
Show resolved
Hide resolved
packages/workflow-core/src/lib/plugins/common-plugin/workflow-token-plugin.ts
Show resolved
Hide resolved
- Add a visual indication for debug mode in the CollectionFlow component - Introduce theme and locales tabs in the UIDefinitionEditor - Utilize dayjs for date formatting in the WorkflowDefinitionSummaryCard (Your code looks nice, but I bet even your cat would prefer a more straightforward UI)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
apps/workflows-dashboard/src/pages/WorkflowDefinition/components/UIDefinitionEditor/UIDefinitionEditor.tsx (2)
34-34: Avoid hardcoding the view mode valueThe
enableViewModeprop is hardcoded totrue, which reduces component reusability. Consider making it configurable through component props.- enableViewMode={true} + enableViewMode={props.enableViewMode}
180-209: Add loading states for JSON editor operationsThe JSON editor operations might be slow with large objects. Consider adding loading states and error boundaries.
const [isEditorLoading, setIsEditorLoading] = useState(false); const handleEditorChange = async (value: any) => { setIsEditorLoading(true); try { await handleUIDefinitionChange(value); } catch (error) { // Handle error } finally { setIsEditorLoading(false); } }; // In JSX <JSONEditorComponent value={uiDefinitionValue} onChange={handleEditorChange} isLoading={isEditorLoading} />apps/kyb-app/src/pages/CollectionFlow/CollectionFlow.tsx (4)
249-258: Add ARIA attributes for better accessibilityThe debug mode indicator's accessibility could be improved by adding appropriate ARIA attributes.
-<div className="mb-6 rounded-lg border border-amber-200 bg-amber-50 p-4"> +<div className="mb-6 rounded-lg border border-amber-200 bg-amber-50 p-4" role="alert" aria-label="Debug mode status"> <div className="mb-4 flex items-center gap-2"> - <div className="h-2 w-2 animate-pulse rounded-full bg-amber-500" /> + <div className="h-2 w-2 animate-pulse rounded-full bg-amber-500" role="presentation" /> <span className="cursor-help font-medium text-amber-900 hover:underline" data-tooltip-id="debug-mode-tooltip" data-tooltip-content="In debug mode you can navigate between steps without validation. Be aware that if required data is missing, plugins may fail when processing data at the end of the flow." + role="button" + tabIndex={0} >
261-272: Consider using a semantic HTML element for state displayThe current state display could benefit from using more semantic HTML elements and adding ARIA attributes for better accessibility.
-<div className="mb-3 text-sm text-amber-800"> +<section className="mb-3 text-sm text-amber-800" aria-label="Current debug state"> - Current State:{' '} + <h4 className="inline">Current State: </h4> {currentPage ? ( - <span className="font-medium"> + <span className="font-medium" role="status" aria-live="polite"> {currentPage.stateName} </span> ) : ( - <span className="italic"> + <span className="italic" role="status" aria-live="polite"> Page not found - state: {state} </span> )} -</div> +</section>
274-285: Enhance debug navigation buttons with keyboard handlingThe debug navigation buttons should handle keyboard events and have proper ARIA labels.
<div className="flex gap-3"> <button onClick={() => stateApi.sendEvent('PREVIOUS')} + onKeyDown={(e) => e.key === 'Enter' && stateApi.sendEvent('PREVIOUS')} className="rounded bg-amber-100 px-3 py-1.5 text-sm font-medium text-amber-900 transition-colors hover:bg-amber-200" + aria-label="Go to previous step in debug mode" > Previous </button> <button onClick={() => stateApi.sendEvent('NEXT')} + onKeyDown={(e) => e.key === 'Enter' && stateApi.sendEvent('NEXT')} className="rounded bg-amber-100 px-3 py-1.5 text-sm font-medium text-amber-900 transition-colors hover:bg-amber-200" + aria-label="Go to next step in debug mode" > Next </button> </div>
249-285: Consider environment-based debug mode controlThe debug mode is currently controlled via localStorage, which might not be the most secure approach. Consider implementing environment-based control for debug mode activation.
Recommendations:
- Add an environment variable (e.g.,
VITE_ENABLE_DEBUG_MODE) to control debug mode availability- Implement a secure mechanism to enable debug mode (e.g., requiring specific credentials)
- Add logging when debug mode is activated to track usage
This would prevent accidental exposure of debug features in production environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/kyb-app/src/pages/CollectionFlow/CollectionFlow.tsx(1 hunks)apps/workflows-dashboard/src/domains/ui-definitions/ui-definitions.types.ts(2 hunks)apps/workflows-dashboard/src/pages/WorkflowDefinition/components/UIDefinitionEditor/UIDefinitionEditor.tsx(1 hunks)apps/workflows-dashboard/src/pages/WorkflowDefinition/components/WorkflowDefinitionSummaryCard/WorkflowDefinitionSummaryCard.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/workflows-dashboard/src/domains/ui-definitions/ui-definitions.types.ts
🔇 Additional comments (9)
apps/workflows-dashboard/src/pages/WorkflowDefinition/components/UIDefinitionEditor/UIDefinitionEditor.tsx (3)
38-38: Replace 'any' type with proper interfaces
Multiple instances of any type usage reduce type safety. Consider creating proper interfaces for your element types.
Also applies to: 60-60, 90-90, 97-97, 124-124, 144-144
62-79: Extract form element rendering logic into separate components
The current implementation has complex nested rendering logic with multiple levels of element traversal. Consider extracting this logic into separate components for better maintainability and reusability.
Also applies to: 81-115
211-220: Enhance error handling and user feedback for async operations
The upgrade and save operations lack proper loading states and error handling.
apps/workflows-dashboard/src/pages/WorkflowDefinition/components/WorkflowDefinitionSummaryCard/WorkflowDefinitionSummaryCard.tsx (5)
15-18: LGTM! Clean and modern card styling
The card header styling with gradient background and shadow provides good visual hierarchy and follows modern design patterns.
20-50: Well-structured data mapping implementation
The array-based approach for rendering workflow details is clean, maintainable, and provides consistent styling.
28-28:
Add null check for date formatting
The current implementation might throw an error if workflowDefinition.createdAt is undefined.
- value: dayjs(workflowDefinition.createdAt).format('MMM D, YYYY h:mm A'),
+ value: workflowDefinition.createdAt
+ ? dayjs(workflowDefinition.createdAt).format('MMM D, YYYY h:mm A')
+ : 'N/A',31-40: 🛠️ Refactor suggestion
Reconsider color coding for public/private status
The current color scheme (red for public, green for private) might be counterintuitive as red typically indicates warning/danger states.
- <span className="rounded-full bg-red-100 px-2 py-1 text-sm font-medium text-red-800">
+ <span className="rounded-full bg-blue-100 px-2 py-1 text-sm font-medium text-blue-800">
Yes
</span>
- <span className="rounded-full bg-green-100 px-2 py-1 text-sm font-medium text-green-800">
+ <span className="rounded-full bg-gray-100 px-2 py-1 text-sm font-medium text-gray-800">
No
</span>23-25: Verify the valueOrNA utility implementation
Let's ensure the valueOrNA utility properly handles all edge cases for these fields.
✅ Verification successful
Let me search for the valueOrFallback implementation since valueOrNA is built on top of it.
Let me check the actual implementation of valueOrFallback to ensure it handles all edge cases properly.
The valueOrNA utility implementation is robust and handles edge cases appropriately
The implementation is well-designed and handles all necessary edge cases:
- Uses
??operator whencheckFalsy: falseto handle only null/undefined - Uses
||operator whencheckFalsy: trueto handle all falsy values (null, undefined, empty string, 0, etc.)
In the case of valueOrNA, it's configured with checkFalsy: true, which means it will properly handle:
- null/undefined values
- Empty strings
- Other falsy values
This is appropriate for the display fields being used (displayName, version, variant) as it ensures a consistent "N/A" display for any missing or empty data.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for valueOrNA implementation and its tests
rg -A 5 "valueOrNA" --type ts
Length of output: 36468
Script:
#!/bin/bash
# Search for valueOrFallback implementation
rg -A 5 "valueOrFallback" --type ts
Length of output: 6484
Script:
#!/bin/bash
# Get the implementation of valueOrFallback
cat packages/common/src/utils/value-or-fallback/value-or-fallback.ts
Length of output: 633
apps/kyb-app/src/pages/CollectionFlow/CollectionFlow.tsx (1)
249-285: Verify debug mode state persistence
The debug mode UI relies on localStorage.getItem('devmode') for visibility control. Let's verify the debug mode state management implementation across the codebase.
...-dashboard/src/pages/WorkflowDefinition/components/UIDefinitionEditor/UIDefinitionEditor.tsx
Show resolved
Hide resolved
- Integrate toast notifications for clipboard copy success and failure - Apply changes across multiple table column components (your clipboard functionality needed a better alert system than just wishing on a star)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
apps/workflows-dashboard/src/pages/UIDefinitions/components/UIDefinitionsTable/columns.tsx (4)
55-59: Add semantic meaning to the UI Context badgeWhile the styling is visually appealing, the badge could benefit from semantic meaning for screen readers.
Apply this diff to improve accessibility:
-<span className="rounded bg-violet-50 px-2 py-1 text-sm text-violet-700"> +<span + className="rounded bg-violet-50 px-2 py-1 text-sm text-violet-700" + role="status" + aria-label={`UI Context: ${info.getValue<string>()}`} +>
62-87: Consider extracting common JSON view patternThe Definition and UI Schema columns share identical implementation patterns. Consider extracting this into a reusable component to follow DRY principles.
Create a new component like this:
const JsonViewCell = ({ json }: { json: unknown }) => ( <div className="flex flex-row items-center gap-3"> <JSONViewButton trigger={ <Eye className="h-5 w-5 cursor-pointer text-gray-600 transition-colors hover:text-blue-600" /> } json={JSON.stringify(json)} /> </div> );Then use it in both columns:
columnHelper.accessor('definition', { - cell: info => ( - <div className="flex flex-row items-center gap-3"> - <JSONViewButton - trigger={ - <Eye className="h-5 w-5 cursor-pointer text-gray-600 transition-colors hover:text-blue-600" /> - } - json={JSON.stringify(info.getValue())} - /> - </div> - ), + cell: info => <JsonViewCell json={info.getValue()} />, header: () => <span className="font-semibold">Definition</span>, }),
88-108: Add type safety to locales handlingWhile the null handling is good, the code could benefit from explicit typing.
Consider this improvement:
-const locales = info.getValue() ? JSON.stringify(info.getValue()) : null; +type Locales = Record<string, unknown> | null; +const locales = info.getValue<Locales>(); +const serializedLocales = locales ? JSON.stringify(locales) : null;
129-131: Enhance navigation link accessibilityThe navigation icon should have an accessible label for screen readers.
Apply this diff:
-<Link to={`/ui-definitions/${info.row.original.id}`} className="flex justify-center"> +<Link + to={`/ui-definitions/${info.row.original.id}`} + className="flex justify-center" + aria-label={`View UI Definition ${info.row.original.id}`} +> <ArrowRightCircleIcon className="h-6 w-6 text-blue-600 transition-colors hover:text-blue-700" /> </Link>apps/workflows-dashboard/src/pages/WorkflowDefinitions/components/molecules/WorkflowDefinitionsTable/columns.tsx (2)
56-68: Extract visibility indicator styling into a reusable componentThe visibility indicator uses conditional styling that could be reused for other boolean indicators.
Create a shared component:
// components/atoms/BooleanIndicator.tsx interface BooleanIndicatorProps { value: boolean; trueText?: string; falseText?: string; trueClassName?: string; falseClassName?: string; } export const BooleanIndicator: React.FC<BooleanIndicatorProps> = ({ value, trueText = 'Public', falseText = 'Private', trueClassName = 'bg-emerald-100 text-emerald-700 hover:bg-emerald-200', falseClassName = 'bg-slate-100 text-slate-700 hover:bg-slate-200' }) => ( <span className={`rounded-full px-3 py-1 text-xs font-medium transition-colors ${ value ? trueClassName : falseClassName }`} > {value ? trueText : falseText} </span> );Then simplify the column implementation:
cell: info => ( - <span - className={`rounded-full px-3 py-1 text-xs font-medium transition-colors ${ - info.getValue<boolean>() - ? 'bg-emerald-100 text-emerald-700 hover:bg-emerald-200' - : 'bg-slate-100 text-slate-700 hover:bg-slate-200' - }`} - > - {info.getValue<boolean>() ? 'Public' : 'Private'} - </span> + <BooleanIndicator value={info.getValue<boolean>()} /> )
Line range hint
1-1: Consider implementing a table column factory patternThere's significant duplication across table column configurations. Consider implementing a factory pattern for common column types:
- ID columns with copy functionality
- JSON viewer columns
- Boolean indicator columns
- Date formatter columns
- Link columns
This would improve maintainability, ensure consistency, and reduce code duplication across the application.
Example implementation:
// utils/table/columnFactory.ts export const createTableColumns = { idColumn: (options?: IdColumnOptions) => ({ cell: info => <IdCell value={info.getValue()} {...options} />, header: () => <span className="font-semibold">ID</span>, }), jsonColumn: (options?: JsonColumnOptions) => ({ cell: info => <JsonViewerCell json={info.getValue()} {...options} />, header: () => <span className="font-semibold">{options?.header ?? 'JSON'}</span>, }), // ... other column factories };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/workflows-dashboard/src/components/molecules/WorkflowsTable/columns.tsx(2 hunks)apps/workflows-dashboard/src/pages/Filters/components/FiltersTable/columns.tsx(1 hunks)apps/workflows-dashboard/src/pages/UIDefinitions/components/UIDefinitionsTable/columns.tsx(1 hunks)apps/workflows-dashboard/src/pages/WorkflowDefinitions/components/molecules/WorkflowDefinitionsTable/columns.tsx(1 hunks)
🔇 Additional comments (4)
apps/workflows-dashboard/src/pages/UIDefinitions/components/UIDefinitionsTable/columns.tsx (3)
18-44: Enhance accessibility of the copy button
The copy button lacks proper accessibility attributes which are essential for screen readers and keyboard navigation.
Apply this diff to improve accessibility:
<button
onClick={() => {
navigator.clipboard
.writeText(info.getValue<string>())
.then(() => {
toast.success('ID copied to clipboard');
})
.catch(() => {
toast.error('Failed to copy ID to clipboard');
});
}}
className="text-gray-400 hover:text-gray-600"
+ aria-label="Copy ID to clipboard"
+ title="Copy ID to clipboard"
+ type="button"
>49-52: LGTM!
The name column implementation is clean and uses appropriate styling for visual hierarchy.
109-114: LGTM!
The implementation handles null cases well using the valueOrNA utility and applies appropriate styling.
apps/workflows-dashboard/src/pages/Filters/components/FiltersTable/columns.tsx (1)
72-86: 🛠️ Refactor suggestion
Improve type safety for query access
The current implementation uses type assertion without validation, which could lead to runtime errors if the query structure is unexpected.
Consider adding proper type definitions and validation:
+interface WorkflowDefinitionQuery {
+ where?: {
+ workflowDefinitionId?: {
+ in?: string[];
+ };
+ };
+}
+const getWorkflowId = (query: unknown): string => {
+ const typedQuery = query as WorkflowDefinitionQuery;
+ return typedQuery?.where?.workflowDefinitionId?.in?.[0] ?? '';
+};
columnHelper.accessor(
- row => {
- const query = row.query as { where?: { workflowDefinitionId?: { in?: string[] } } };
- return query.where?.workflowDefinitionId?.in?.[0] ?? '';
- },
+ row => getWorkflowId(row.query),
{
id: 'workflowDefinitionId',
// ... rest of the implementation
}
)Likely invalid or redundant comment.
apps/workflows-dashboard/src/pages/Filters/components/FiltersTable/columns.tsx
Show resolved
Hide resolved
apps/workflows-dashboard/src/components/molecules/WorkflowsTable/columns.tsx
Show resolved
Hide resolved
- Update version for backoffice-v2, kyb-app, and workflows-service to 0.7.80 - Upgrade workflow-browser-sdk and workflow-node-sdk to version 0.6.76 (your dependencies have more updates than my social media feed during a trend)
- Replace detailed inline object definitions with separate schemas for individuals and businesses - Enhance readability and maintainability by utilizing existing schemas (Your code reduction skills are sharp, just like the corners of the new schema)
- Introduce a Dialog component for viewing the integration catalog - Implement state management for opening and closing the dialog - Add copy-to-clipboard functionality for vendor keys (Your integration catalog is so fancy, it makes a five-star restaurant look like a food truck)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
apps/workflows-dashboard/src/pages/WorkflowDefinition/WorkflowDefinition.tsx (2)
21-54: Consider moving vendor details to a configuration serviceThe hardcoded vendor details could be better managed through a configuration service for the following reasons:
- Environment-specific URLs can be managed more effectively
- Vendor information can be updated without code changes
- Sensitive vendor information can be properly secured
Consider:
- Moving vendor details to a configuration service or environment variables
- Implementing a caching mechanism for vendor logos
- Adding URL validation for logo URLs
275-286: Reduce code duplication in image error handlingThe image error handling logic is duplicated across multiple instances.
Extract to a reusable component:
interface VendorLogoProps { vendor: string; className?: string; } const VendorLogo: React.FC<VendorLogoProps> = ({ vendor, className }) => { const fallbackLogo = 'https://cdn.ballerine.io/logos/ballerine-logo.png'; const logoUrl = VENDOR_DETAILS[vendor as VendorId]?.logoUrl || `https://cdn.ballerine.io/logos/${vendor.toLowerCase()}-logo.png`; return ( <img src={logoUrl} alt={vendor} className={className} onError={e => { e.currentTarget.src = fallbackLogo; }} /> ); };Also applies to: 282-285
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/workflows-dashboard/src/pages/WorkflowDefinition/WorkflowDefinition.tsx(3 hunks)
🔇 Additional comments (1)
apps/workflows-dashboard/src/pages/WorkflowDefinition/WorkflowDefinition.tsx (1)
Line range hint 1-371: Verify relationship with filter creation functionality
While the file contains significant UI enhancements for workflow definition management, it's unclear how these changes relate to the PR's primary objective of implementing filter creation functionality.
Let's verify the connection:
apps/workflows-dashboard/src/pages/WorkflowDefinition/WorkflowDefinition.tsx
Outdated
Show resolved
Hide resolved
apps/workflows-dashboard/src/pages/WorkflowDefinition/WorkflowDefinition.tsx
Show resolved
Hide resolved
apps/workflows-dashboard/src/pages/WorkflowDefinition/WorkflowDefinition.tsx
Show resolved
Hide resolved
- Update version numbers in various packages to their latest releases - Upgrade dependent packages to maintain compatibility (your projects are like a fine wine, they really should age before a release)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
examples/report-generation-example/CHANGELOG.md (1)
3-10: Enhance changelog entry quality and commit message format.The changelog entries could be more informative by:
- Following conventional commit format (feat/fix/chore etc.)
- Including brief descriptions of actual changes
- Documenting any breaking changes or migration steps if applicable
- Explaining the reason for dependency updates
Example of a better format:
-## 0.2.26 - -### Patch Changes - -- Bump -- Updated dependencies - - @ballerine/react-pdf-toolkit@1.2.50 +## 0.2.26 (2024-12-XX) + +### Patch Changes + +- chore(deps): update @ballerine/react-pdf-toolkit to 1.2.50 + - Includes improvements in PDF rendering performance + - No breaking changessdks/workflow-browser-sdk/CHANGELOG.md (2)
7-7: Consider providing more descriptive changelog messages.The message "Bump" is too vague and doesn't provide meaningful context about the changes. Consider following the Conventional Commits format and including a brief description of the actual changes.
Example of a better message:
-Bump +chore(deps): update workflow-core and common dependencies
3-10: Consider standardizing the changelog format.The changelog entries are inconsistent in their formatting. Some entries use "bump", others use "version bump", and some just list dependency updates. Consider adopting a consistent format across all entries.
Example of a consistent format:
## [0.6.77] - YYYY-MM-DD ### Changes - Update @ballerine/workflow-core to 0.6.77 - Update @ballerine/common to 0.9.58 ### Description Brief description of notable changes or the reason for the version bumpapps/workflows-dashboard/src/pages/WorkflowDefinition/WorkflowDefinition.tsx (1)
20-51: Consider moving vendor details to a configuration fileThe
VENDOR_DETAILSconstant contains vendor-specific information that might need frequent updates. Consider moving this to a separate configuration file for better maintainability and easier updates.Create a new file
config/vendors.ts:export const VENDOR_DETAILS = { 'dow-jones': { logoUrl: 'https://cdn.ballerine.io/logos/Dow_Jones_Logo.png', description: 'Dow Jones provides sanctions screening and risk data for individuals', }, // ... rest of the vendor details } as const; export type VendorId = keyof typeof VENDOR_DETAILS;Then import it in this file:
-export const VENDOR_DETAILS = { - // ... vendor details -} as const; - -export type VendorId = keyof typeof VENDOR_DETAILS; +import { VENDOR_DETAILS, VendorId } from '@/config/vendors';packages/react-pdf-toolkit/CHANGELOG.md (1)
7-7: Enhance changelog message clarity.The message "Bump" is too vague. Please provide a more descriptive message that explains what changed in this version. This helps other developers understand the purpose of this release.
Example of a better message:
-Bump +Updated dependencies to support new filter creation functionality
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
apps/backoffice-v2/CHANGELOG.md(1 hunks)apps/backoffice-v2/package.json(3 hunks)apps/kyb-app/CHANGELOG.md(1 hunks)apps/kyb-app/package.json(3 hunks)apps/workflows-dashboard/CHANGELOG.md(1 hunks)apps/workflows-dashboard/package.json(3 hunks)apps/workflows-dashboard/src/pages/WorkflowDefinition/WorkflowDefinition.tsx(3 hunks)examples/headless-example/CHANGELOG.md(1 hunks)examples/headless-example/package.json(2 hunks)examples/report-generation-example/CHANGELOG.md(1 hunks)examples/report-generation-example/package.json(2 hunks)packages/blocks/CHANGELOG.md(1 hunks)packages/blocks/package.json(3 hunks)packages/common/CHANGELOG.md(1 hunks)packages/common/package.json(2 hunks)packages/config/CHANGELOG.md(1 hunks)packages/config/package.json(1 hunks)packages/eslint-config-react/CHANGELOG.md(1 hunks)packages/eslint-config-react/package.json(2 hunks)packages/eslint-config/CHANGELOG.md(1 hunks)packages/eslint-config/package.json(1 hunks)packages/react-pdf-toolkit/CHANGELOG.md(1 hunks)packages/react-pdf-toolkit/package.json(2 hunks)packages/rules-engine/CHANGELOG.md(1 hunks)packages/rules-engine/package.json(2 hunks)packages/ui/CHANGELOG.md(1 hunks)packages/ui/package.json(3 hunks)packages/workflow-core/CHANGELOG.md(1 hunks)packages/workflow-core/package.json(3 hunks)sdks/web-ui-sdk/CHANGELOG.md(1 hunks)sdks/web-ui-sdk/package.json(2 hunks)sdks/workflow-browser-sdk/CHANGELOG.md(1 hunks)sdks/workflow-browser-sdk/package.json(2 hunks)sdks/workflow-node-sdk/CHANGELOG.md(1 hunks)sdks/workflow-node-sdk/package.json(2 hunks)services/websocket-service/CHANGELOG.md(1 hunks)services/websocket-service/package.json(1 hunks)services/workflows-service/CHANGELOG.md(1 hunks)services/workflows-service/package.json(3 hunks)services/workflows-service/prisma/data-migrations(1 hunks)websites/docs/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (32)
- packages/config/package.json
- packages/eslint-config/package.json
- services/websocket-service/package.json
- packages/common/CHANGELOG.md
- packages/eslint-config-react/CHANGELOG.md
- sdks/web-ui-sdk/CHANGELOG.md
- packages/rules-engine/CHANGELOG.md
- examples/report-generation-example/package.json
- packages/workflow-core/CHANGELOG.md
- packages/eslint-config-react/package.json
- services/workflows-service/CHANGELOG.md
- packages/ui/CHANGELOG.md
- packages/blocks/CHANGELOG.md
- apps/workflows-dashboard/CHANGELOG.md
- packages/eslint-config/CHANGELOG.md
- packages/config/CHANGELOG.md
- sdks/workflow-node-sdk/CHANGELOG.md
- packages/common/package.json
- packages/react-pdf-toolkit/package.json
- apps/backoffice-v2/CHANGELOG.md
- apps/kyb-app/CHANGELOG.md
- examples/headless-example/package.json
- examples/headless-example/CHANGELOG.md
- sdks/workflow-node-sdk/package.json
- packages/rules-engine/package.json
- packages/workflow-core/package.json
- packages/blocks/package.json
- services/websocket-service/CHANGELOG.md
- services/workflows-service/package.json
- apps/workflows-dashboard/package.json
- apps/backoffice-v2/package.json
- apps/kyb-app/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/prisma/data-migrations
🔇 Additional comments (10)
websites/docs/package.json (1)
20-20: LGTM! Consider documenting new features.
The dependency version updates maintain consistency across the monorepo. However, since this PR implements new filter functionality, please ensure the documentation is updated accordingly.
Let's verify if documentation for the new filter functionality exists:
Also applies to: 26-27
✅ Verification successful
Documentation is up to date with filter functionality
The documentation already comprehensively covers filters functionality, including:
- Detailed API endpoints for creating and managing filters
- Clear explanation of filters as case queues in the case management system
- Usage of filters in the dashboard UI
- Examples of filter implementation in both KYC and KYB workflows
The version updates in the dependencies appear to be routine maintenance and don't introduce new filter functionality that would require documentation updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if filter-related documentation exists and if it needs updating
# Search for filter-related documentation files
echo "Searching for filter-related documentation..."
fd -e md -e mdx . websites/docs/src/content/docs/ --exec grep -l -i "filter"
# Search for any TODO comments related to filters in documentation
echo "Checking for TODO comments related to filters..."
rg -i "todo.*filter" websites/docs/src/content/docs/
Length of output: 715
Script:
#!/bin/bash
# Let's check the content of these files to see if they already document the new filter functionality
# or if they need updates
echo "Checking content of filter-related documentation files..."
for file in \
"websites/docs/src/content/docs/en/learn/add_and_customize_workflows_in_the_case_management.md" \
"websites/docs/src/content/docs/en/learn/using_the_case_management_dashboard.md" \
"websites/docs/src/content/docs/en/learn/kyc_manual_review_example.mdx" \
"websites/docs/src/content/docs/en/learn/kyb_manual_review_example.mdx" \
"websites/docs/src/content/docs/en/learn/invoking_a_workflow.md"
do
echo "=== Content of $file ==="
cat "$file" | grep -A 5 -B 5 -i "filter"
done
# Let's also check what's new in the common package that might need documentation
echo "Checking recent changes in @ballerine/common package..."
rg -l "filter" packages/common/src/ | xargs -I {} echo "=== {}" && rg "filter" packages/common/src/
Length of output: 9192
apps/workflows-dashboard/src/pages/WorkflowDefinition/WorkflowDefinition.tsx (4)
65-67: Enhance clipboard functionality with error handling and user feedback
The clipboard function should handle potential errors and provide user feedback.
164-262: Extract integration catalog dialog to a separate component
The dialog implementation is quite large and contains complex nested logic.
279-279: Remove any type casting for better type safety
Define proper interfaces for the plugin type instead of using any.
300-304: LGTM! Well-implemented image error handling
The error handling for image loading with a fallback to the default logo is well implemented.
packages/react-pdf-toolkit/CHANGELOG.md (1)
8-10: Verify dependency updates align with filter creation feature.
Let's ensure these dependency updates support the new filter creation functionality mentioned in the PR objectives.
sdks/workflow-browser-sdk/package.json (1)
4-4: Verify version consistency across the monorepo
The version updates look consistent, but let's verify the version alignment across the monorepo to ensure all packages are properly synchronized.
Also applies to: 36-37, 44-44, 46-46
✅ Verification successful
Version alignment verified across the monorepo
All @ballerine packages show consistent version numbers across the monorepo:
- @ballerine/common: 0.9.58
- @ballerine/workflow-core: 0.6.77
- @ballerine/config: 1.1.27
- @ballerine/eslint-config: 1.1.27
The version updates in workflow-browser-sdk/package.json are properly synchronized with all other packages in the monorepo.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check version consistency across @ballerine packages
# Find all package.json files and extract @ballerine/* dependency versions
echo "Checking @ballerine/* dependency versions across the monorepo..."
rg -g "package.json" "@ballerine/(?:common|workflow-core|config|eslint-config).*\d+\.\d+\.\d+" -A 1
Length of output: 5211
packages/ui/package.json (1)
4-4: Verify the version jump in @ballerine/common dependency
The @ballerine/common dependency is being updated from ^0.9.55 to ^0.9.58, skipping versions 0.9.56 and 0.9.57. Let's verify if there are any breaking changes or important updates in the skipped versions that need to be considered.
Also applies to: 29-29, 68-69
✅ Verification successful
Based on the output from the shell scripts, I can now generate a final response. The CHANGELOG.md entries show that versions 0.9.56 through 0.9.58 of @ballerine/common were regular package updates without any breaking changes. The changes appear to be routine package upgrades across the monorepo's packages.
Version updates in @ballerine/common are routine package upgrades
The version jump from 0.9.55 to 0.9.58 consists of regular package updates without any breaking changes. The CHANGELOG.md entries confirm these were standard upgrades propagated across all dependent packages in the monorepo.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in @ballerine/common between versions
# Look for BREAKING CHANGE or BREAKING CHANGES in commit messages
echo "Checking for breaking changes in @ballerine/common..."
git log --grep="BREAKING CHANGE\|BREAKING CHANGES" -i --since="1 month ago"
# Check changelog entries if they exist
echo "Checking changelog entries..."
rg -g "CHANGELOG.md" "## \[0\.9\.(56|57)\]" -A 10
Length of output: 235
Script:
#!/bin/bash
# Let's try a different approach to check the version changes
# Check package.json in @ballerine/common for version history
echo "Checking @ballerine/common package.json versions..."
fd "package.json" --exec grep -l "@ballerine/common" {} \; | xargs cat
# Look for any recent changes in @ballerine/common
echo "Checking recent changes in common package..."
fd "package.json" -p "packages/common/" --exec cat {}
# Check for any migration guides or upgrade notes
echo "Checking for migration/upgrade documentation..."
rg -g "*.md" -i "upgrade|migration|breaking|0.9.5[6-8]"
Length of output: 53346
sdks/web-ui-sdk/package.json (2)
24-24: LGTM! Version updates are consistent
The version bump and dependency update are minimal and align with the changes in other packages.
Also applies to: 99-99
Line range hint 1-999: Note: Package updates don't implement filter functionality
While these version updates are necessary for maintaining dependency consistency, they don't directly contribute to the PR's objective of implementing create filter functionality. Are there additional files that contain the actual filter implementation?
* feat(filters): implement create filter functionality (#2881) * style(components): change button state styling to fix disappearing buttons (#2877) * style(components): enhance button state styling for better UX - Update button classes to include hover and enabled states - Remove unused CSS rules to streamline styles (your buttons now have more states than a soap opera character) * fix(buttons): update background color for button components - Change button styles from slate to primary background and hover colors - Ensure consistency across different components for better UX (your color choices are so dark, they could be the backdrop for a horror movie) --------- Co-authored-by: Omri Levy <61207713+Omri-Levy@users.noreply.github.com> * fix(stepper): improve step display and clean up formatting - Refactor step display to enhance layout and readability - Add a new no-op constant to built-in events - Clean up transition validation logic (Your transition validation is so confusing, it makes find-and-replace look like a clear path) * Date input improvements (#2889) * refactor(*): changed handling of date inputs * ci(*): testing path change * temporarily disabled test * updated hook name --------- Co-authored-by: Tomer Shvadron <tomers@ballerine.com> * EditableDetailsV2 input type improvement (#2891) * refactor(*): changed handling of date inputs * ci(*): testing path change * temporarily disabled test * updated hook name * fix(backoffice-v2): no longer looking at form value for input type --------- Co-authored-by: Tomer Shvadron <tomers@ballerine.com> * EditableDetailsV2 added ability to override input type (#2892) * refactor(*): changed handling of date inputs * ci(*): testing path change * temporarily disabled test * updated hook name * fix(backoffice-v2): no longer looking at form value for input type * feat(backoffice-v2): added a way to override input type --------- Co-authored-by: Tomer Shvadron <tomers@ballerine.com> * Dev 318/action alert delivery (#2893) * feat: adding changes for sending alerts to specific channel * fix: added change in hotfix action --------- Co-authored-by: Alon Peretz <8467965+alonp99@users.noreply.github.com> Co-authored-by: Shane <66246046+shanegrouber@users.noreply.github.com> Co-authored-by: Tomer Shvadron <tomers@ballerine.com> Co-authored-by: Chirag <134513193+codechirag123@users.noreply.github.com>
(Your filter creation process is so smooth, it's practically butter on a hot knife)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style
Tests
Chores