Illiar/feat/app state #2796
Illiar/feat/app state #2796
Conversation
|
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several modifications across various components and services related to state management and context handling within the application. Key changes include updates to the 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: 33
🧹 Outside diff range and nitpick comments (38)
packages/common/src/utils/collection-flow-manager/schemas/config-schema.ts (2)
23-23: Add TSDoc comment for the exported type.Consider adding documentation to improve code maintainability:
+/** + * Configuration type for the Collection Flow Manager. + * This type is derived from the CollectionFlowManagerConfigSchema and used for type checking. + */ export type CollectionFlowManagerConfig = Static<typeof CollectionFlowManagerConfigSchema>;
1-23: Consider enhancing schema extensibility and reusability.To improve the architecture:
- Consider extracting common schema patterns into shared utilities
- Add schema versioning support for future compatibility
- Consider implementing a schema registry pattern for managing multiple related schemas
- Add schema validation error handling utilities
This will make the schema system more maintainable and extensible as the collection flow system grows.
packages/common/src/index.ts (2)
Line range hint
4-34: Consider grouping related utility exports together.The utility exports could be better organized by grouping related functions together. For example:
- Type checking utilities:
isFunction,isInstanceOfFunction,isType, etc.- String/URL validation:
checkIsUrl,checkIsIsoDate- Error handling:
isErrorWithCode,isErrorWithMessageConsider reorganizing the exports alphabetically within their functional groups for better maintainability.
Line range hint
1-64: Consider splitting exports into separate barrel files.The file is handling multiple concerns (utils, types, schemas, etc.). Consider splitting these into separate barrel files (e.g.,
utils/index.ts,types/index.ts) for better organization and maintenance.This would:
- Improve code organization
- Make it easier to manage dependencies
- Reduce the risk of circular dependencies
- Make it clearer what's being exported from each module
packages/common/src/schemas/documents/collection-flow-context-schema.ts (1)
4-11: Consider improving enum naming consistency and documentation.The enum values are descriptive, but there are a few suggestions for improvement:
- 'inprogress' should be 'inProgress' to follow camelCase convention
- Consider adding JSDoc comments to document:
- The purpose of each state
- Valid state transitions
- When each state is triggered
Here's the suggested improvement:
export enum CollectionFlowStateEnum { pending = 'pending', - inprogress = 'inprogress', + inProgress = 'inProgress', approved = 'approved', rejected = 'rejected', failed = 'failed', revision = 'revision', }packages/common/src/utils/collection-flow-manager/helpers/config-helper.ts (1)
1-43: Consider architectural improvements for better maintainability.
- Consider implementing a logging interface that can be injected:
interface Logger { log(message: string, ...args: any[]): void; }
Consider implementing an observer pattern or integrating with a state management system to properly track configuration changes.
Consider adding a schema validation library like Zod or Joi for runtime type checking of configuration values.
Would you like assistance in implementing any of these architectural improvements?
apps/kyb-app/src/components/organisms/DynamicUI/hooks/useUIStateLogic/hooks/useUIElementsStateLogic/hooks/useUIElementToolsLogic/useUIElementToolsLogic.ts (3)
9-10: Consider adding type annotations for better type safety.The destructured values from
useStateManagerContextand thepayloadRefwould benefit from explicit type annotations.- const { payload, stateApi } = useStateManagerContext(); - const payloadRef = useRefValue(payload); + const { payload, stateApi } = useStateManagerContext<CollectionFlowContext>(); + const payloadRef = useRefValue<typeof payload>(payload);
Line range hint
24-30: Verify the necessity of new dependencies.The
setElementCompletedcallback haspayloadRefandstateApiin its dependency array, but these values don't appear to be used within the function body. This could indicate:
- Missing functionality that should use these values
- Unnecessary dependencies that could trigger extra re-renders
Either:
- Add the missing functionality that uses these dependencies, or
- Remove them from the dependency array if they're not needed:
}, - [elementsStateRef, payloadRef, stateApi, setUIElementState], + [elementsStateRef, setUIElementState],
Line range hint
6-41: Consider optimizing state updates and ref usage.The hook uses multiple refs and state updates which could be optimized:
elementsStateRefcould potentially be replaced withuseRefValue- State updates could be batched if multiple elements are being updated
Consider this optimization:
export const useUIElementToolsLogic = (elementId: string) => { const { helpers, state } = useDynamicUIContext(); const { setUIElementState } = helpers; const { payload, stateApi } = useStateManagerContext(); const payloadRef = useRefValue(payload); - const elementsStateRef = useRef(state.elements); + const elementsStateRef = useRefValue(state.elements); - useEffect(() => { - elementsStateRef.current = state.elements; - }, [state.elements]); // ... rest of the codeapps/kyb-app/src/components/organisms/DynamicUI/StateManager/StateManager.tsx (1)
23-23: Document the state structure change.The change in state structure from
flowConfig.appStatetocollectionFlow.state.uiStaterepresents a significant architectural change that should be documented.Add a comment explaining the state structure:
+ // State is now managed through CollectionFlow context instead of flowConfig + // to provide better integration with collection-specific state management state: initialContext?.collectionFlow?.state?.uiState,packages/common/src/utils/collection-flow-manager/helpers/config-helper.unit.test.ts (4)
5-12: Consider strengthening the basic instantiation test.While the test confirms the helper is defined, it could be more robust by:
- Verifying the initial state of properties
- Using a proper type factory instead of type assertion
Consider this improvement:
test('should be defined with initial state', () => { const context: CollectionFlowContextSchema = { collectionFlow: { config: {} } }; const configHelper = new ConfigHelper(context); expect(configHelper).toBeDefined(); expect(configHelper.apiUrl).toBeUndefined(); expect(configHelper.tokenId).toBeUndefined(); });
14-26: Enhance apiUrl test coverage with edge cases.The current test verifies basic get/set functionality, but consider adding tests for:
- Invalid URL formats
- Empty string or undefined values
- URL normalization (if applicable)
Example additional test cases:
test('should handle invalid apiUrl values', () => { const configHelper = new ConfigHelper({ collectionFlow: { config: {} } } as CollectionFlowContextSchema); // Test empty string configHelper.apiUrl = ''; expect(configHelper.apiUrl).toBe(''); // Test undefined configHelper.apiUrl = undefined; expect(configHelper.apiUrl).toBeUndefined(); // Test invalid URL (if validation is required) configHelper.apiUrl = 'not-a-url'; // Assert expected behavior });
28-40: Add validation tests for tokenId.Consider adding tests to verify:
- Token format validation (if specific format is required)
- Empty string or undefined handling
- Token length constraints
Example additional test cases:
test('should handle edge cases for tokenId', () => { const configHelper = new ConfigHelper({ collectionFlow: { config: {} } } as CollectionFlowContextSchema); // Test empty string configHelper.tokenId = ''; expect(configHelper.tokenId).toBe(''); // Test undefined configHelper.tokenId = undefined; expect(configHelper.tokenId).toBeUndefined(); // Test token format (if validation is required) const validToken = '1234567890'; configHelper.tokenId = validToken; expect(configHelper.tokenId).toBe(validToken); });
42-62: Add tests for partial config override scenarios.The current test verifies a complete override, but consider testing:
- Partial overrides (only apiUrl or only tokenId)
- Override with undefined values
- Multiple consecutive overrides
Example additional test:
test('should handle partial override', () => { const ctx = { collectionFlow: { config: { apiUrl: 'https://api.example.com', tokenId: '1234567890' } } } as CollectionFlowContextSchema; const configHelper = new ConfigHelper(ctx); // Test partial override configHelper.override({ apiUrl: 'https://new-api.example.com' }); expect(configHelper.apiUrl).toBe('https://new-api.example.com'); expect(configHelper.tokenId).toBe('1234567890'); // Test undefined values configHelper.override({ tokenId: undefined }); expect(configHelper.tokenId).toBeUndefined(); });apps/kyb-app/src/components/organisms/DynamicUI/StateManager/hooks/useMachineLogic/useMachineLogic.ts (1)
Line range hint
31-56: Consider updating context type casting in sendEvent methodThe
sendEventmethod still usesAnyObjectfor context casting, while the rest of the file has been updated to useCollectionFlowContext. This inconsistency might lead to type-related issues.Consider updating the type casting:
- const context = machine.getSnapshot().context as AnyObject; + const context = machine.getSnapshot().context as CollectionFlowContext; machine.overrideContext({ ...context, flowConfig: { - ...(context.flowConfig as AnyObject), + ...(context.flowConfig), appState: nextStateName, }, });apps/kyb-app/src/components/organisms/DynamicUI/StateManager/components/ActionsHandler/action-handlers/value-apply/value-apply.handler.ts (2)
Line range hint
16-42: Consider revising the state management approach.The current implementation both mutates state through
api.setContext()and returns a context, which could lead to race conditions in concurrent scenarios. Consider either:
- Making the handler pure by returning the new context without calling
setContext- Making the handler responsible for state management only, returning void
Example of approach #1:
async run<TContext>( _: TContext, action: Action<ValueApplyParams>, api: StateMachineAPI, ): Promise<TContext> { const context = structuredClone(api.getContext()); action?.params?.values?.forEach(params => { // ... existing logic ... }); return Promise.resolve(context); }Example of approach #2:
async run<TContext>( _: TContext, action: Action<ValueApplyParams>, api: StateMachineAPI, ): Promise<void> { const context = structuredClone(api.getContext()); action?.params?.values?.forEach(params => { // ... existing logic ... }); api.setContext(context); return Promise.resolve(); }
Based on the results, I can now generate the final response as we have clear evidence about the context types being used in the codebase.
The
TContextgeneric type in value-apply handler should be replaced withCollectionFlowContextThe codebase shows that:
- The entire application uses
CollectionFlowContexttype (imported from@/domains/collection-flow/types/flow-context.types)- Other action handlers like
EventDispatcherHandlerandPluginRunnerHandlerare already usingCollectionFlowContext- The
value-apply.handler.tsis the only handler still using the genericTContexttypeThe handler's signature should be updated to:
async run( _: CollectionFlowContext, action: Action<ValueApplyParams>, api: StateMachineAPI, ): Promise<CollectionFlowContext>🔗 Analysis chain
Line range hint
16-42: Verify the impact of context type changes across the codebase.Given the codebase-wide migration from generic
TContexttoCollectionFlowContext, ensure that this handler's generic type parameter aligns with the new type system.
Let me analyze the results and generate additional verification scripts to check for the specific context type being used in the state machine and related components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of context type usage across action handlers # Test 1: Check other action handlers' run method signatures echo "Checking action handler implementations..." ast-grep --pattern 'class $_ implements ActionHandler { $$$ run<$_>($_: $_, $_: Action<$_>, $_: $_): Promise<$_> { $$$ } }' # Test 2: Check for any remaining generic TContext usage echo "Checking for generic TContext usage..." rg "TContext" --type tsLength of output: 11677
Script:
#!/bin/bash # Check for CollectionFlowContext usage and its definition echo "Checking CollectionFlowContext usage..." rg "CollectionFlowContext" --type ts -A 2 # Check for ActionHandler interface definition to understand the expected context type echo "Checking ActionHandler interface definition..." ast-grep --pattern 'interface ActionHandler { $$$ }' # Check other value-apply related files for context type usage echo "Checking value-apply related files..." fd "value-apply" --type f --exec rg "TContext|CollectionFlowContext" --type ts {}Length of output: 30989
apps/kyb-app/src/components/organisms/DynamicUI/StateManager/hooks/useStateLogic/useStateLogic.ts (2)
Line range hint
43-56: Fix potential race condition in context updatesThe
contextRefupdate insidesetStatecould lead to race conditions if React batches multiple state updates. Consider updating the ref before calling setState:const setContext = useCallback( (newContext: CollectionFlowContext) => { const newCtx = { ...newContext }; machineApi.setContext(newCtx); + const nextState = { payload: newCtx, machineState: prev.machineState, isPluginLoading: prev.isPluginLoading }; + contextRef.current = nextState; setState(prev => { - const nextState = { ...prev, payload: newCtx }; - contextRef.current = nextState; - return nextState; }); return newCtx; }, [machineApi, contextRef], );
Line range hint
60-73: Standardize error handling and loading state managementThe
invokePluginfunction lacks error handling and uses a different loading state approach compared tosendEvent. Consider standardizing the implementation:const invokePlugin = useCallback( async (pluginName: string) => { + helpers.setLoading(true); setState(prev => ({ ...prev, isPluginLoading: true })); - await machineApi.invokePlugin(pluginName); + try { + await machineApi.invokePlugin(pluginName); - setState({ - machineState: machineApi.getState(), - payload: machineApi.getContext(), - isPluginLoading: false, - }); + setState({ + machineState: machineApi.getState(), + payload: machineApi.getContext(), + isPluginLoading: false, + }); + } catch (error) { + console.error( + `Error occurred while invoking plugin ${pluginName}`, + isErrorWithMessage(error) ? error.message : error, + ); + throw error; + } finally { + helpers.setLoading(false); + setState(prev => ({ ...prev, isPluginLoading: false })); + } }, - [machineApi], + [machineApi, helpers], );packages/common/src/utils/collection-flow-manager/helpers/state-helper.unit.test.ts (4)
15-31: Enhance error handling test coverage.While the basic error cases are covered, consider adding tests for:
- Null/undefined collectionFlow
- Null/undefined state within collectionFlow
- Invalid state structure
describe('state helper will throw', () => { test('if collectionFlow is null', () => { expect(() => new StateHelper({ collectionFlow: null } as CollectionFlowContextSchema)).toThrow(); }); test('if state is null', () => { expect(() => new StateHelper({ collectionFlow: { state: null } } as CollectionFlowContextSchema)).toThrow(); }); });
32-53: Improve test readability and type handling.The test cases are good but could benefit from:
- More descriptive test names that explain the expected behavior
- A shared test fixture to reduce type assertion complexity
- test('if uiState is in progress', () => { + test('should allow updating uiState when valid steps exist in progress', () => {Also, consider extracting the test context into a shared fixture:
const createTestContext = (progress: Record<string, { isCompleted: boolean }>) => ({ collectionFlow: { state: { uiState: 'step1', progress } } } as CollectionFlowContextSchema);
54-81: Add comprehensive state validation tests.Consider adding tests for:
- Each possible enum value
- State transitions between different enum values
- State persistence after updates
test.each(Object.values(CollectionFlowStateEnum))('should accept valid state: %s', (state) => { const stateHelper = new StateHelper({ collectionFlow: { state: {} } } as CollectionFlowContextSchema); stateHelper.collectionFlowState = state; expect(stateHelper.collectionFlowState).toBe(state); });
82-97: Expand override method test coverage.The current test only covers a basic case. Consider adding tests for:
- Partial overrides (only uiState or only collectionFlowState)
- Invalid override values
- Multiple consecutive overrides
describe('override', () => { test('should allow partial override of uiState only', () => { // Test implementation }); test('should allow partial override of collectionFlowState only', () => { // Test implementation }); test('should maintain consistency across multiple overrides', () => { // Test implementation }); });apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/JSONForm.tsx (1)
Line range hint
77-85: Address technical debt in form data transformation logic.The TODO comment indicates that this logic should be moved to jmespath. Additionally, the current implementation:
- Mutates the payload directly using
set- Contains multiple type assertions and potential type safety issues
- Handles business logic inside a UI component
Consider:
- Moving this transformation logic to a dedicated service/helper
- Using jmespath for consistent data access patterns
- Implementing proper type guards instead of type assertions
Would you like help creating a separate service for handling these transformations?
services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts (3)
63-63: LGTM! Consider adding error handling.The change improves the architecture by delegating context retrieval to the
CollectionFlowService, which aligns with the single responsibility principle and the PR's goal of enhancing state management.Consider adding error handling to gracefully handle cases where context retrieval fails:
@common.Get('/context') async getContext(@TokenScope() tokenScope: ITokenScope) { - return this.service.getCollectionFlowContext(tokenScope); + try { + return await this.service.getCollectionFlowContext(tokenScope); + } catch (error) { + throw new common.InternalServerErrorException('Failed to retrieve collection flow context'); + } }
Line range hint
91-105: Remove or implement the commented-out method.Since this PR is marked as WIP, please either:
- Implement the
updateFlowConfigurationmethod if it's needed- Remove the commented code if it's no longer required
Line range hint
146-190: Consider abstracting common event handling logic.Multiple methods (
updateContextById,finishFlow,resubmitFlow) share similar event handling patterns usingworkflowService.event(). Consider creating a helper method to reduce code duplication:private async emitWorkflowEvent( tokenScope: ITokenScope, eventName: string, payload?: Record<string, unknown>, ) { return this.workflowService.event( { id: tokenScope.workflowRuntimeDataId, name: eventName, payload, }, [tokenScope.projectId], tokenScope.projectId, ); }This would simplify the event-emitting methods:
@common.Post('/send-event') async finishFlow(@TokenScope() tokenScope: ITokenScope, @common.Body() body: FinishFlowDto) { return this.emitWorkflowEvent(tokenScope, body.eventName); } @common.Post('resubmit') async resubmitFlow(@TokenScope() tokenScope: ITokenScope) { return this.emitWorkflowEvent(tokenScope, 'RESUBMITTED'); }packages/common/src/utils/collection-flow-manager/collection-flow-manager.unit.test.ts (3)
12-16: Enhance invalid config test cases.The current test only checks one generic invalid case. Consider adding specific test cases for different types of invalid configurations to better document the requirements.
// Example additional test cases: it('fails with missing apiUrl', () => { expect(() => new CollectionFlowManager({} as CollectionFlowContextSchema, { tokenId: '1234', steps: [] } as any)).toThrow('Invalid collection flow manager config.'); }); it('fails with missing steps', () => { expect(() => new CollectionFlowManager({} as CollectionFlowContextSchema, { apiUrl: 'https://api.example.com', tokenId: '1234' } as any)).toThrow('Invalid collection flow manager config.'); });
32-40: Add edge cases for flow state tests.Consider adding test cases for:
- Flow with undefined state
- Flow with empty state object
- Flow with invalid uiState value
test('when state is undefined', () => { const manager = new CollectionFlowManager({ collectionFlow: {} } as CollectionFlowContextSchema); expect(manager.isStarted()).toBe(false); }); test('when uiState is invalid', () => { const manager = new CollectionFlowManager({ collectionFlow: { state: { uiState: 'invalid_step' } } } as CollectionFlowContextSchema); expect(() => manager.isStarted()).toThrow(); });
42-75: Enhance context validation in update tests.The current tests verify basic properties but should also validate:
- Complete state structure
- Steps initialization
- Default values
Add these assertions to the existing test:
expect(context.collectionFlow?.state).toEqual({ uiState: undefined, progress: { step1: { isCompleted: false, } } }); expect(context.collectionFlow?.config?.steps).toHaveLength(1);packages/common/src/utils/collection-flow-manager/helpers/state-helper.ts (4)
22-23: Implement a proper logging mechanism instead ofconsole.logUsing
console.logis not recommended for production code as it lacks control over log levels and outputs. Consider using a logging library likewinstonorbunyanto provide better logging practices.Apply this diff to replace
console.logwith a logging utility:- console.log(`UI State transition from ${this.uiState} to ${uiState}`); + logger.info(`UI State transition from ${this.uiState} to ${uiState}`);Ensure you import and configure the logger at the beginning of your file:
+ import logger from 'path-to-your-logger';
39-41: Replaceconsole.logwith a structured logging solutionConsistent with previous logging suggestions, replace
console.logwith a logging library to handle different log levels and outputs effectively.Apply this diff to update the logging statement:
- console.log( + logger.info( `Collection flow state transition from ${this.collectionFlowState} to ${collectionFlowState}`, );
67-68: Utilize a logging library instead ofconsole.logstatementsFor consistent logging, replace
console.logwith your chosen logging utility.- console.log(`Step: ${step} isCompleted: ${isCompleted}`); + logger.info(`Step: ${step} isCompleted: ${isCompleted}`);
80-80: Switch to a logging framework for state override notificationsAs with previous instances, replace
console.logwith your logging utility to maintain consistent logging practices.- console.log('StateHelper, context override', this.context.collectionFlow!.state); + logger.info('StateHelper, context override', this.context.collectionFlow?.state);apps/kyb-app/src/pages/CollectionFlow/CollectionFlow.tsx (1)
133-135: Replaceconsole.logstatements with a proper logging mechanismUsing
console.login production code is discouraged as it may expose sensitive information and is not suitable for error tracking or monitoring.Consider using a dedicated logging library or framework that allows you to manage log levels and outputs appropriately.
services/workflows-service/src/workflow/workflow.service.ts (3)
Line range hint
2099-2112: Clarify Error Messages for MissingballerineFileIdIn the
documentFetchPagesContentPromise, whenballerineFileIdis missing, the thrown error message is "Cannot run document OCR on document without pages," which may not accurately reflect the issue. Provide a more specific error message to aid in debugging and improve clarity.Apply this diff to improve the error message:
if (!ballerineFileId) { - throw new BadRequestException('Cannot run document OCR on document without pages'); + throw new BadRequestException('Document page is missing a valid ballerineFileId'); }
Line range hint
2086-2145: Ensure Robust Exception Handling inrunOCROnDocumentMethodThe
runOCROnDocumentmethod performs multiple asynchronous operations and relies on external services. Confirm that all potential exceptions are properly handled, and consider adding try-catch blocks where necessary to prevent unhandled exceptions and to provide informative error responses.
1469-1487: ValidateuiSchema.elementsBefore InitializingCollectionFlowManagerEnsure that
uiSchema.elementsis properly defined and valid before using it to initializeCollectionFlowManager. Adding validation checks can prevent potential runtime errors ifuiSchema.elementsis undefined or malformed.
📜 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 (34)
- apps/kyb-app/src/components/organisms/DynamicUI/StateManager/StateManager.tsx (1 hunks)
- apps/kyb-app/src/components/organisms/DynamicUI/StateManager/components/ActionsHandler/action-handlers/action-handler.abstract.ts (1 hunks)
- apps/kyb-app/src/components/organisms/DynamicUI/StateManager/components/ActionsHandler/action-handlers/event-dispatcher.handler.ts (2 hunks)
- apps/kyb-app/src/components/organisms/DynamicUI/StateManager/components/ActionsHandler/action-handlers/plugin-runner.handler.ts (2 hunks)
- apps/kyb-app/src/components/organisms/DynamicUI/StateManager/components/ActionsHandler/action-handlers/value-apply/value-apply.handler.ts (1 hunks)
- apps/kyb-app/src/components/organisms/DynamicUI/StateManager/hooks/useMachineLogic/useMachineLogic.ts (3 hunks)
- apps/kyb-app/src/components/organisms/DynamicUI/StateManager/hooks/useStateLogic/useStateLogic.ts (1 hunks)
- apps/kyb-app/src/components/organisms/DynamicUI/StateManager/types.ts (1 hunks)
- apps/kyb-app/src/components/organisms/DynamicUI/hooks/useUIStateLogic/hooks/useUIElementsStateLogic/hooks/useUIElementToolsLogic/useUIElementToolsLogic.ts (2 hunks)
- apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/JSONForm.tsx (1 hunks)
- apps/kyb-app/src/domains/collection-flow/collection-flow.api.ts (2 hunks)
- apps/kyb-app/src/domains/collection-flow/types/flow-context.types.ts (1 hunks)
- apps/kyb-app/src/domains/collection-flow/types/index.ts (2 hunks)
- apps/kyb-app/src/helpers/prepareInitialUIState.ts (2 hunks)
- apps/kyb-app/src/pages/CollectionFlow/CollectionFlow.tsx (3 hunks)
- packages/common/package.json (2 hunks)
- packages/common/rollup.config.ts (5 hunks)
- packages/common/src/index.ts (2 hunks)
- packages/common/src/schemas/documents/collection-flow-context-schema.ts (1 hunks)
- packages/common/src/schemas/documents/default-context-schema.ts (1 hunks)
- packages/common/src/schemas/index.ts (2 hunks)
- packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/collection-flow-manager.unit.test.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/helpers/config-helper.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/helpers/config-helper.unit.test.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/helpers/state-helper.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/helpers/state-helper.unit.test.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/index.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/schemas/config-schema.ts (1 hunks)
- packages/common/vitest.config.ts (1 hunks)
- services/workflows-service/prisma/data-migrations (1 hunks)
- services/workflows-service/src/collection-flow/collection-flow.service.ts (2 hunks)
- services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts (1 hunks)
- services/workflows-service/src/workflow/workflow.service.ts (6 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/common/src/utils/collection-flow-manager/index.ts
- services/workflows-service/prisma/data-migrations
🧰 Additional context used
🔇 Additional comments (48)
apps/kyb-app/src/domains/collection-flow/types/flow-context.types.ts (2)
Line range hint
6-12: LGTM! Interfaces maintain backward compatibility.The unchanged
CollectionFlowConfigandCollectionFlowContextDatainterfaces properly reference the newCollectionFlowContexttype while maintaining their existing structure. This helps ensure a smoother migration path.
2-2: Verify the migration to CollectionFlowContextSchema across the codebase.The change to use
CollectionFlowContextSchemafrom@ballerine/commonis a significant architectural improvement that centralizes the schema definition. However, this is a breaking change that requires verification of all consumers.Let's verify the schema usage across the codebase:
Also applies to: 4-4
✅ Verification successful
Based on the shell script results, I can now provide a final response:
Migration to CollectionFlowContextSchema is properly implemented across the codebase
The verification shows that:
- The schema is consistently imported from
@ballerine/commonacross all relevant files- All files using
CollectionFlowContexthave been properly updated to use the new schema- The old context properties usage (
appState,customerCompany) are not found in the codebase- The only remaining
flowConfigandstepsProgressusages are in:
- UI definition scripts (template strings)
- Web UI SDK loading component
- Submit button component (setting progress state)
These usages are valid and expected as they represent either configuration templates or UI-specific functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all files using CollectionFlowContext are properly updated # Test 1: Find all files that import or use CollectionFlowContext echo "=== Files importing or using CollectionFlowContext ===" rg -l "CollectionFlowContext" # Test 2: Check for any remaining direct usage of the old context properties echo -e "\n=== Potential usage of old context properties ===" rg "flowConfig\." rg "appState\." rg "customerCompany\." rg "stepsProgress\." # Test 3: Verify all imports of the new schema echo -e "\n=== Files importing CollectionFlowContextSchema ===" rg "CollectionFlowContextSchema"Length of output: 15474
apps/kyb-app/src/components/organisms/DynamicUI/StateManager/components/ActionsHandler/action-handlers/action-handler.abstract.ts (1)
3-3: LGTM!Clean addition of the
CollectionFlowContextimport, maintaining proper import organization.apps/kyb-app/src/components/organisms/DynamicUI/StateManager/components/ActionsHandler/action-handlers/plugin-runner.handler.ts (2)
4-4: LGTM! Good improvement in type safety.The explicit import and use of
CollectionFlowContextinstead of a generic type enhances type safety and maintains consistency with the broader state management refactor.
13-13: Verify the impact of removing generic context type.The change from generic
TContextto specificCollectionFlowContextis a breaking change. While this aligns with the standardization effort, we should verify that all plugin runners are compatible with this context type.apps/kyb-app/src/components/organisms/DynamicUI/StateManager/components/ActionsHandler/action-handlers/event-dispatcher.handler.ts (2)
4-4: LGTM!Clean import that aligns with the standardization of CollectionFlowContext usage across the codebase.
Line range hint
13-18: Verify StateMachineAPI contract compatibility.The method signature change from generic type to CollectionFlowContext looks good. However, let's verify the StateMachineAPI contract to ensure type compatibility.
✅ Verification successful
Let me try a different approach to verify the StateMachineAPI contract.
Based on the search results, I can now provide a definitive response about the StateMachineAPI contract compatibility:
StateMachineAPI contract is fully compatible with the changes
The interface definition in
useMachineLogic.tsshows that:
getContext()returnsCollectionFlowContextsendEvent(eventName: string)accepts only the event name parameterThe event-dispatcher handler's implementation correctly:
- Uses
CollectionFlowContextas the context type- Calls
api.sendEvent(eventName)with just the event name- Returns the context via
api.getContext()All type signatures align perfectly with the StateMachineAPI interface contract.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify StateMachineAPI interface compatibility with CollectionFlowContext # Test 1: Check if StateMachineAPI.getContext returns CollectionFlowContext ast-grep --pattern 'interface StateMachineAPI { $$$ getContext(): $_ $$$ }' # Test 2: Check if sendEvent method accepts context parameter in its signature ast-grep --pattern 'interface StateMachineAPI { $$$ sendEvent($_): $_ $$$ }'Length of output: 168
Script:
#!/bin/bash # Search for StateMachineAPI interface definition rg "interface StateMachineAPI" -A 10 # Also search for any type definitions or implementations related to StateMachineAPI rg "StateMachineAPI" -A 3 -B 3Length of output: 23417
apps/kyb-app/src/components/organisms/DynamicUI/StateManager/types.ts (1)
14-14: LGTM! Type safety improvement noted.The change from
AnyObjecttoCollectionFlowContextenhances type safety and aligns with the existing usage ofCollectionFlowContextinStateManagerProps. This is a positive change that makes the types more specific and maintainable.Let's verify the impact on consuming code:
✅ Verification successful
Let's gather more specific information about how the
StateManagerContextis being used in these files.The previous search showed many files using
StateManagerContext, but we need to understand:
- How the context is consumed (destructuring, direct access)
- What type annotations are used with payload
- Whether there are any existing type constraints
This will help us verify if the type change is safe and compatible with existing usage patterns.
Let's search for more specific information about the
CollectionFlowContexttype and its usage.This will help us understand:
- The definition and structure of
CollectionFlowContext- How the state manager context is consumed and used
- How payload updates are handled in the state management system
This information will help us verify if the type change from
AnyObjecttoCollectionFlowContextis safe and compatible with existing usage patterns.
Based on the gathered information, I can now provide a final response about the type change from
AnyObjecttoCollectionFlowContext.Type change is safe and consistent with the codebase
The change from
AnyObjecttoCollectionFlowContextfor thepayloadproperty inStateManagerContextis safe and well-aligned with the codebase for the following reasons:
CollectionFlowContextis already the expected type throughout the codebase:
- It's used in the state manager's hooks (
useStateLogic.ts,useMachineLogic.ts)- The API returns
CollectionFlowContext(collection-flow.api.ts)- Components like
JSONFormandStepperUIexpectCollectionFlowContextThe type is properly defined:
- It's imported from
@ballerine/commonasCollectionFlowContextSchema- It's re-exported in the domain types as
CollectionFlowContextAll consumers are already handling this type:
- State updates in
useStateLogic.tsuseCollectionFlowContext- Action handlers expect
CollectionFlowContext- The machine context is typed as
CollectionFlowContextThe change actually makes the type more accurate by removing the overly permissive
AnyObjecttype, matching the actual runtime type that's being used throughout the application.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of StateManagerContext to ensure they can handle CollectionFlowContext # Search for files importing or using StateManagerContext rg -l "StateManagerContext" # Search for direct access to the payload property rg -A 3 "context\.payload"Length of output: 2581
Script:
#!/bin/bash # Search for usage patterns of StateManagerContext to understand how payload is accessed ast-grep --pattern 'useContext($state)' --pattern-type ts ast-grep --pattern 'const { payload } = $expr' --pattern-type ts ast-grep --pattern 'payload: $expr' --pattern-type ts # Look for specific imports of StateManagerContext rg "import.*StateManagerContext" -A 1 # Search for direct payload access patterns rg "payload\s*[:=]" --type tsLength of output: 24230
Script:
#!/bin/bash # Search for CollectionFlowContext definition and usage rg "CollectionFlowContext" -A 3 # Search for specific state manager context usage patterns rg "useStateManagerContext.*=.*" -A 2 # Look for state updates in the codebase rg "setState.*payload" -A 2Length of output: 38543
apps/kyb-app/src/helpers/prepareInitialUIState.ts (2)
4-4: LGTM: Import statement is correctly placed and utilized.
23-23: Document the early return condition and verify state management.The purpose of the early return condition isn't immediately clear. Consider adding a comment explaining why we return an empty state when the first page's state name matches the UI state.
Let's verify how this state management pattern is used across the codebase:
✅ Verification successful
Let me gather more context about the state management pattern by examining the related files.
Based on the gathered context, I can now generate the final response since I have enough information about the state management pattern across the codebase.
Early return is valid but needs documentation for clarity
The early return condition is part of a consistent state management pattern used across the codebase where:
CollectionFlowManagertracks the UI state throughcollectionFlow?.state?.uiStateStateManagercomponent uses this state for machine initialization- The early return in
prepareInitialUIStatespecifically handles the initial state when we're at the first page, avoiding unnecessary element initializationConsider adding a comment like:
// Return empty initial state when we're still at the first page to avoid premature element initialization if (pages[0]?.stateName === context.collectionFlow?.state?.uiState) return initialUIState;🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for similar state management patterns and potential inconsistencies # Search for similar state access patterns rg -l "collectionFlow\?\.state\?\.uiState" # Look for other early returns based on state ast-grep --pattern 'if ($condition) return $state'Length of output: 302
Script:
#!/bin/bash # Check the implementation in collection-flow-manager rg -A 10 "collectionFlow\?\.state\?\.uiState" packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts # Check the StateManager component implementation rg -A 10 "collectionFlow\?\.state\?\.uiState" apps/kyb-app/src/components/organisms/DynamicUI/StateManager/StateManager.tsx # Look at the full context of prepareInitialUIState cat apps/kyb-app/src/helpers/prepareInitialUIState.tsLength of output: 1926
packages/common/src/index.ts (2)
42-52: Verify type compatibility with removed types.The removal of
ObjectValuestype (as mentioned in the AI summary) while addingCollectionFlowContextSchemasuggests a shift in type organization.Let's verify there are no breaking changes:
#!/bin/bash # Check for any remaining usage of the removed ObjectValues type rg "ObjectValues[^a-zA-Z]" # Look for potential type dependencies rg "CollectionFlowContextSchema"
37-38: Verify the new collection-flow-manager export usage.The addition of this new module export suggests significant changes to collection flow management.
Let's verify the usage and potential impact:
✅ Verification successful
Let me gather more specific information about the collection-flow-manager usage and implementation.
The previous results show that CollectionFlowContextSchema is used in multiple places, but we need to understand the actual implementation and usage of the collection-flow-manager to verify its impact.
Based on the implementation details gathered, I can now provide a final response:
The collection-flow-manager export is a safe addition with minimal impact
The collection-flow-manager is a self-contained utility class that manages collection flow state and configuration. It's designed to be opt-in, meaning:
- It requires explicit initialization via the
start()method- It maintains its own isolated context
- It's only imported in packages/common/src/index.ts and has no direct consumers in the codebase yet
The implementation shows it's a new feature addition rather than a modification of existing functionality, making it safe to export.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing imports of collection-flow-manager rg -l "from ['\"].*\/collection-flow-manager['\"]" # Check the implementation to understand the exported functionality ast-grep --pattern 'export * from "./utils/collection-flow-manager"'Length of output: 16126
Script:
#!/bin/bash # Check for direct imports of collection-flow-manager rg "from.*collection-flow-manager['\"]" # Check the implementation of collection-flow-manager fd "collection-flow-manager.ts" --exec cat {} # Check for any tests that might indicate the functionality fd "collection-flow-manager.test.ts" --exec cat {} fd "collection-flow-manager.spec.ts" --exec cat {}Length of output: 3601
packages/common/src/schemas/index.ts (2)
12-12: LGTM!The re-organization of existing exports maintains consistency and doesn't introduce any breaking changes.
Also applies to: 13-16, 26-26
1-5: Verify the integration of new CollectionFlow exports.The new exports look good and follow proper TypeScript naming conventions. However, let's verify that these new types are being used consistently across the codebase.
✅ Verification successful
Based on the shell script results, I can now generate the final response as we have comprehensive information about the usage of these new exports across the codebase.
New CollectionFlow exports are properly integrated and used consistently
The new exports are well-integrated across the codebase with proper usage patterns:
CollectionFlowStateEnumis used consistently in:
- Collection flow manager utilities for state management
- Unit tests validating state transitions
- KYB app's CollectionFlow component for rendering different states
CollectionFlowContextSchemais properly utilized in:
- Workflow service for context management
- Collection flow manager and its helper classes
- Type definitions in the KYB app
collectionFlowContextSchemaserves as the base schema definition and is correctly imported where neededThe exports follow a clean architecture pattern where the schema definitions are centralized and consumed appropriately across different layers of the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new CollectionFlow types across the codebase # Expected: Find references to these types in components and services echo "Checking CollectionFlowStateEnum usage:" rg "CollectionFlowStateEnum" -A 2 echo "Checking CollectionFlowContextSchema usage:" rg "CollectionFlowContextSchema" -A 2 echo "Checking collectionFlowContextSchema usage:" rg "collectionFlowContextSchema" -A 2Length of output: 24645
packages/common/src/schemas/documents/collection-flow-context-schema.ts (3)
1-2: LGTM! Appropriate use of TypeBox for schema validation.The imports are well-structured, utilizing TypeBox for runtime type validation while extending the default context schema.
43-43: LGTM! Proper type export using TypeBox Static utility.The type export correctly derives the TypeScript type from the schema definition.
1-43: Verify schema integration with dependent components.The schema is referenced by multiple components mentioned in the summary. Let's verify the integration:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Schema and enum integration is properly maintained across the codebase
The verification shows that:
- The schema and enum are correctly imported and used across multiple components in packages/common, services/workflows-service, and apps/kyb-app
- All enum values are properly referenced using
CollectionFlowStateEnumin the codebase- While there are hardcoded string literals matching enum values in workflow definitions and service layers, this is expected as they represent different domain concepts (workflow states vs UI states)
The schema integration appears to be consistent and well-maintained, with no issues found in the dependent components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of CollectionFlowContextSchema and CollectionFlowStateEnum # Test 1: Check imports of the schema echo "Checking schema imports..." rg -l "CollectionFlowContextSchema|CollectionFlowStateEnum" # Test 2: Verify correct enum value usage echo "Checking enum value usage..." rg "CollectionFlowStateEnum\.[a-zA-Z]+" -A 1 # Test 3: Check for any hardcoded strings that should use the enum echo "Checking for potential missed enum usage..." rg "'(pending|inprogress|approved|rejected|failed|revision)'" --type tsLength of output: 22211
apps/kyb-app/src/components/organisms/DynamicUI/hooks/useUIStateLogic/hooks/useUIElementsStateLogic/hooks/useUIElementToolsLogic/useUIElementToolsLogic.ts (1)
2-3: Verify import dependencies to prevent circular references.The new imports from the StateManager and hooks directory could potentially create circular dependencies.
✅ Verification successful
Let me gather more information about the import structure to make a more informed decision.
Based on the results, I can now generate the final response:
The import structure is safe and does not create circular dependencies
The analysis of the codebase shows that:
- The StateProvider is a simple context provider that doesn't import from the hooks directory
- The useRefValue is a standalone utility hook with no dependencies on DynamicUI components
- The only reference to useUIElementToolsLogic is from TransitionListener.tsx which is a consumer, not a dependency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential circular dependencies in the import chain # Expected: No circular references between StateManager, hooks, and current file # Test 1: Check if StateManager imports from current directory rg -l "from.*useUIElementToolsLogic" --glob "apps/kyb-app/src/components/organisms/DynamicUI/StateManager/**" # Test 2: Check if useRefValue hook has dependencies on current directory rg -l "from.*useUIElementToolsLogic" --glob "apps/kyb-app/src/hooks/**"Length of output: 185
Script:
#!/bin/bash # Let's check the actual content of these files to understand the dependency structure # Check the StateManager context provider implementation echo "=== StateManager Context Implementation ===" fd "StateProvider.ts" apps/kyb-app/src/components/organisms/DynamicUI/StateManager/ --exec cat {} # Check the useRefValue hook implementation echo -e "\n=== useRefValue Hook Implementation ===" fd "useRefValue.ts" apps/kyb-app/src/hooks/ --exec cat {} # Check if there are any other imports of useUIElementToolsLogic in the DynamicUI folder echo -e "\n=== References to useUIElementToolsLogic in DynamicUI ===" rg "from.*useUIElementToolsLogic" apps/kyb-app/src/components/organisms/DynamicUI/Length of output: 1687
apps/kyb-app/src/domains/collection-flow/collection-flow.api.ts (2)
10-10: LGTM! Import changes align with the new context structure.The import statement correctly reflects the simplified context structure, removing unused types and importing only the necessary
CollectionFlowContext.
67-71: Verify the impact of return type changes.The function's return type has changed from containing both
contextandconfigto directly returningCollectionFlowContext. This is a breaking change that could affect existing consumers.Let's verify the usage of this function:
packages/common/src/schemas/documents/default-context-schema.ts (2)
Line range hint
15-43: LGTM! Schema structure remains unchanged.The schema definitions (
defaultInputContextSchema,defaultContextSchema, andDefaultContextSchematype) are well-structured and maintain their existing functionality while supporting the broader CollectionFlowContext integration.
4-4: Verify schema import paths after reorganization.The import statements have been reorganized, with some schemas moved to a new location. Let's verify that all imports are correctly resolved after this reorganization.
Also applies to: 7-10, 12-12
✅ Verification successful
Since all the imported schema files are found in their expected locations under
packages/common/src/schemas/documents/schemas/, I can now generate the final response.Import paths are correctly organized and resolved ✓
All schema files referenced in the import statements exist in their new locations under the
schemas/subdirectory:
- business-information-plugin-schema.ts
- aml-schema.ts
- documents-schema.ts
- entity-schema.ts
- kyc-session-plugin-schema.ts
- ubo-plugin-schema.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all imported schema files exist in their new locations # Test: Check if all schema files exist echo "Verifying schema file locations..." fd --type f --extension ts "business-information-plugin-schema.ts|aml-schema.ts|documents-schema.ts|entity-schema.ts|kyc-session-plugin-schema.ts|ubo-plugin-schema.ts" --exec echo "Found: {}"Length of output: 741
apps/kyb-app/src/components/organisms/DynamicUI/StateManager/StateManager.tsx (1)
23-23:⚠️ Potential issueVerify state migration and add type safety checks.
The state access pattern has changed from
flowConfig.appStatetocollectionFlow.state.uiState. This could lead to undefined states if the migration isn't complete.Consider adding type safety:
- state: initialContext?.collectionFlow?.state?.uiState, + state: initialContext?.collectionFlow?.state?.uiState ?? {},✅ Verification successful
Let me try a different search strategy to verify the state migration patterns.
This should give us a better understanding of:
- The context around both state patterns
- Whether there's a mix of old and new patterns
- The type definitions that govern these structures
Based on the search results, I can now provide a final response:
State migration is part of a larger CollectionFlow context refactoring
The change from
flowConfig.appStatetocollectionFlow.state.uiStateis part of a well-structured migration to use CollectionFlowContext throughout the codebase. The search results show:
The new pattern is consistently used across the codebase with proper type definitions and validation:
CollectionFlowManagerclass handles state managementStateHelperclass provides type-safe state access- Proper interfaces and schemas are defined in the codebase
The old
flowConfigpattern is only used for API URL configurations and tokens, not for state management anymore.The state access is protected by the
StateHelperclass which includes null checks and validation, making the optional chaining in the code appropriate for the initialization phase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all components have migrated to the new state structure # and no components are still trying to access flowConfig.appState # Check for old state access pattern echo "Checking for old state access pattern..." rg -l "flowConfig\.appState" # Check for new state access pattern adoption echo "Checking for new state access pattern adoption..." rg -l "collectionFlow\.state\.uiState"Length of output: 271
Script:
#!/bin/bash # Search for any references to flowConfig and collectionFlow in the codebase echo "Searching for flowConfig references..." rg "flowConfig" -A 2 -B 2 echo -e "\nSearching for collectionFlow references..." rg "collectionFlow" -A 2 -B 2 # Also check for any TypeScript interfaces or types related to these echo -e "\nSearching for related type definitions..." ast-grep --pattern 'interface $_ { $$$ flowConfig?: $_ $$$ }' ast-grep --pattern 'interface $_ { $$$ collectionFlow?: $_ $$$ }'Length of output: 105712
packages/common/src/utils/collection-flow-manager/helpers/config-helper.unit.test.ts (1)
1-4: LGTM! Imports are well-organized.The necessary imports are correctly defined, including the schema type and testing utilities.
apps/kyb-app/src/components/organisms/DynamicUI/StateManager/hooks/useMachineLogic/useMachineLogic.ts (5)
1-2: LGTM! Import changes align with type safety improvementsThe addition of
CollectionFlowContextimport and reorganization of imports enhance type safety and maintain good import ordering.
10-11: LGTM! Enhanced type safety in API interfaceThe change from
AnyObjecttoCollectionFlowContextimproves type safety and makes the API contract more explicit.
Line range hint
59-64: LGTM! Consistent type implementation in setContextThe implementation correctly uses the
CollectionFlowContexttype and maintains a clean, straightforward implementation.
72-72: LGTM! Consistent type casting in getContextThe type casting to
CollectionFlowContextmaintains consistency with the interface definition and enhances type safety.
1-1: Verify compatibility with hook consumersThe change from
AnyObjecttoCollectionFlowContextis a breaking change that might affect consumers of this hook.Let's verify the usage:
Also applies to: 10-11
packages/common/package.json (2)
46-46: LGTM: Valid addition of @rollup/plugin-json.The addition of @rollup/plugin-json is appropriate for handling JSON imports in the rollup configuration.
50-50: LGTM: Proper type definitions for crypto-js.The @types/crypto-js version (^4.2.2) correctly aligns with the runtime dependency [email protected].
apps/kyb-app/src/components/organisms/DynamicUI/StateManager/hooks/useStateLogic/useStateLogic.ts (1)
43-50: Improve type safety for context initializationWhile the change to
CollectionFlowContexttype improves type safety, the initialization could be strengthened:
- The empty object default for
initialContextmight not satisfyCollectionFlowContextrequirements- The type assertion when setting context (
as CollectionFlowContext) could mask type errorsLet's verify the structure of CollectionFlowContext:
apps/kyb-app/src/domains/collection-flow/types/index.ts (2)
5-5: Verify the imported type's implementation.The import of
CollectionFlowConfigis part of the broader context management changes. Let's verify its implementation.✅ Verification successful
Let me generate better scripts to verify the imported type's implementation.
Let me try different commands without typescript file type specification since it's not recognized.
Import statement is correctly implemented and used
The verification shows that:
CollectionFlowConfigis properly defined inflow-context.types.tsas an interface with an optionaluiOptionsproperty- The import is correctly used in
index.tswhereUISchemaConfigextends it- The type is also used consistently in the
CollectionFlowContextDatainterface🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the CollectionFlowConfig type definition and its usage # Expected: Find the type definition and other potential usages # Check the type definition echo "Checking CollectionFlowConfig definition:" rg -t typescript "export.*CollectionFlowConfig" # Check other usages echo -e "\nChecking other usages:" rg -t typescript "CollectionFlowConfig[^.]" -A 2Length of output: 382
Script:
#!/bin/bash # Check the type definition in the relative path echo "Checking CollectionFlowConfig in flow-context.types.ts:" rg -t typescript "export.*CollectionFlowConfig" apps/kyb-app/src/domains/collection-flow/types/flow-context.types.ts # Check the content of the types directory echo -e "\nListing files in the types directory:" ls -la apps/kyb-app/src/domains/collection-flow/types/ # Check all usages of CollectionFlowConfig echo -e "\nChecking all usages of CollectionFlowConfig:" rg -t typescript "CollectionFlowConfig" apps/kyb-app/src/domains/collection-flow/types/Length of output: 958
Script:
#!/bin/bash # Check the content of flow-context.types.ts echo "Checking content of flow-context.types.ts:" cat apps/kyb-app/src/domains/collection-flow/types/flow-context.types.ts # Check all usages of CollectionFlowConfig in the types directory echo -e "\nChecking all usages of CollectionFlowConfig:" rg "CollectionFlowConfig" apps/kyb-app/src/domains/collection-flow/types/Length of output: 1167
136-139: Interface extension looks good but verify implementations.The extension of
CollectionFlowConfigaligns with the PR's goal of enhancing type safety and context management. However, since this is a breaking change, we should verify all implementations.✅ Verification successful
Let me try a different approach to find the implementations with corrected syntax.
Let me try one more search with corrected file type and more specific patterns:
Based on the search results, I can now provide a final response:
Interface extension is safe and already properly implemented
The search results show that
UISchemaConfigis only used in two ways:
- As a type definition in
apps/kyb-app/src/domains/collection-flow/types/index.ts- As part of the
UISchemainterface where it's used as a type for theconfigpropertyThe
supportedLanguagesproperty is already being used correctly in:
- The workflows service configuration (
services/workflows-service/scripts/workflows/dynamic-ui-workflow.ts)- The language picker component (
apps/kyb-app/src/components/layouts/AppShell/LanguagePicker.tsx)No breaking changes or implementation updates are needed as the existing code already conforms to the interface requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all implementations of UISchemaConfig to ensure they're updated # Expected: Locate all places where UISchemaConfig is instantiated # Find direct usage of UISchemaConfig echo "Checking UISchemaConfig implementations:" rg -t typescript "UISchemaConfig.*=.*{" -A 5 # Find interface implementations echo -e "\nChecking interface implementations:" ast-grep --pattern 'const $_ : UISchemaConfig = $_'Length of output: 382
Script:
#!/bin/bash # Search for UISchemaConfig usage with proper regex escaping echo "Checking UISchemaConfig usage:" rg -t typescript "UISchemaConfig" -B 2 -A 2 # Search for type annotations and assignments echo -e "\nChecking type annotations:" ast-grep --pattern '$_: UISchemaConfig' # Search for object literals that might implement the interface echo -e "\nChecking object assignments:" ast-grep --pattern 'const $_ = { kybOnExitAction: $_, supportedLanguages: $_ }'Length of output: 3142
Script:
#!/bin/bash # Search for UISchemaConfig in TypeScript and JavaScript files echo "Checking UISchemaConfig usage:" rg --type-add 'ts:*.{ts,tsx}' --type ts "UISchemaConfig" -B 2 -A 2 # Search specifically in the collection-flow directory echo -e "\nChecking collection-flow directory:" fd . apps/kyb-app/src/domains/collection-flow -t f -e ts -e tsx -x rg "UISchemaConfig" -B 2 -A 2 {} # Search for config objects that might implement the interface echo -e "\nChecking potential implementations:" rg --type-add 'ts:*.{ts,tsx}' --type ts "supportedLanguages.*\[" -B 2 -A 2Length of output: 3606
packages/common/src/utils/collection-flow-manager/helpers/state-helper.unit.test.ts (1)
1-5: LGTM! Clean and focused imports.The imports are well-organized and include all necessary dependencies for testing.
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/JSONForm.tsx (1)
81-83:⚠️ Potential issueImprove type safety and null checks in company information handling.
The current implementation has several areas that could be improved:
- The type assertion
(payload as CollectionFlowContext)is unsafe and could lead to runtime errors if the payload doesn't match the expected type.- The nested optional chaining could benefit from a more robust default value strategy.
Consider applying these improvements:
- companyName: get(payload, 'entity.data.companyName', '') as string, - customerCompany: (payload as CollectionFlowContext).collectionFlow - ?.additionalInformation?.customerCompany, + companyName: get(payload, 'entity.data.companyName', '') as string, + customerCompany: get(payload, 'collectionFlow.additionalInformation.customerCompany', null),Let's verify the CollectionFlowContext type usage across the codebase:
✅ Verification successful
Let me gather more information about the CollectionFlowContext type definition and its usage to make a more informed decision.
The previous results show that CollectionFlowContext is used consistently across the codebase, but we need to understand its type definition and any existing type guards to make a proper recommendation about type safety.
Let me check one more thing to understand the type definition of CollectionFlowContext in the codebase.
This will help us understand how CollectionFlowContext is defined and if there are any existing type guards that we should be using instead of type assertions.
Based on the gathered information, I can now provide the final response:
Type assertion is appropriate, but safer access pattern recommended
The type assertion to
CollectionFlowContextis actually valid here since:
CollectionFlowContextis properly defined as an alias toCollectionFlowContextSchemafrom@ballerine/common- The type is consistently used across the codebase for context handling
- The payload comes from a context that's already typed as
CollectionFlowContextHowever, the suggested improvement to use
get()for nested property access is still valuable for runtime safety:customerCompany: get(payload, 'collectionFlow.additionalInformation.customerCompany', null),This maintains type safety while providing better null/undefined handling at runtime.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent CollectionFlowContext type usage # Expected: Find other locations where payload is cast to CollectionFlowContext # Search for CollectionFlowContext type assertions rg "as CollectionFlowContext" -A 2 # Search for CollectionFlowContext type usage in type definitions ast-grep --pattern "type $_ = $$$CollectionFlowContext$$$"Length of output: 10468
Script:
#!/bin/bash # Search for CollectionFlowContext type definition ast-grep --pattern "type CollectionFlowContext = $_" # Search for CollectionFlowContext interface definition ast-grep --pattern "interface CollectionFlowContext $_" # Search for imports of CollectionFlowContext rg "import.*CollectionFlowContext.*from" -A 1 # Search for type guards or type checks for CollectionFlowContext rg "isCollectionFlowContext|CollectionFlowContext\)" -A 2Length of output: 7491
Script:
#!/bin/bash # Get the content of the flow-context.types.ts file cat apps/kyb-app/src/domains/collection-flow/types/flow-context.types.ts # Check if there are any type guards in the codebase rg "function is.*CollectionFlow" -A 3Length of output: 487
packages/common/rollup.config.ts (2)
8-8: LGTM: JSON plugin import added correctly.The import statement for the JSON plugin is properly placed with other plugin imports.
110-110: LGTM: JSON plugin consistently integrated across build formats.The
json()plugin is correctly added to all relevant build configurations (ESM, CJS, UMD Dev, and UMD Prod), ensuring JSON files can be imported as modules across different output formats.Let's verify the JSON plugin usage in the codebase:
Also applies to: 133-133, 156-156, 185-185
✅ Verification successful
Based on the results, I can now generate the final response:
JSON plugin is justified by multiple JSON imports across the codebase
The verification confirms that the JSON plugin is necessary as there are multiple JSON imports throughout the codebase, including:
- Translation files (
.../translation.json)- Package configuration (
package.json)- Country/nationality data from
i18n-iso-countriesandi18n-nationality- Theme configuration (
theme.json)- Toast configurations and other localization files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for JSON imports in the codebase to validate the need for @rollup/plugin-json # Test: Search for JSON imports. Expect: Find .json file imports that justify the plugin addition rg -t ts -t js "from ['\"].*\.json['\"]|import.*\.json['\"]"Length of output: 1873
packages/common/src/utils/collection-flow-manager/collection-flow-manager.unit.test.ts (1)
1-158: Verify test coverage for CollectionFlowManager.Let's ensure we have adequate test coverage for the CollectionFlowManager class.
✅ Verification successful
Let me analyze the test coverage by searching for the implementation of CollectionFlowManager.
Let me search for the actual implementation file to analyze the methods and their test coverage.
Let me try a different approach to view the implementation file directly.
Based on the implementation and test files, I can now provide a final response about the test coverage.
Test coverage for CollectionFlowManager is comprehensive and adequate.
The test suite covers all public methods and critical functionality:
Constructor and initialization
- Basic instantiation
- Invalid config handling
isStarted()method
- Verification of started state based on uiState
start()method
- Success cases with config
- Failure case when already started
- Context mutation verification
- Progress state initialization
config()method
- Success case with proper initialization
- Failure case when config is not set
state()method
- Success case with proper initialization
- Failure case when state is not set
The only method not explicitly tested is
updateAdditionalInformation(), but this is a simple setter method with logging, and its functionality is implicitly tested through the context manipulation tests.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all public methods in CollectionFlowManager echo "Searching for public methods in CollectionFlowManager..." ast-grep --pattern 'class CollectionFlowManager { $$$ $method($_) { $$$ } $$$ }' # Search for error conditions in the implementation echo "Searching for error conditions that might need testing..." rg "throw|Error" --type tsLength of output: 149604
Script:
#!/bin/bash # Search for CollectionFlowManager implementation echo "Searching for CollectionFlowManager implementation..." ast-grep --pattern 'class CollectionFlowManager { $$$ }' # Search for CollectionFlowManager interface/type definitions echo "Searching for CollectionFlowManager interface/type definitions..." rg "interface.*CollectionFlowManager|type.*CollectionFlowManager" --type tsLength of output: 563
Script:
#!/bin/bash # Search for the implementation file echo "Searching for CollectionFlowManager implementation file..." fd "collection-flow-manager.ts" --type f # If found, let's look at its content echo "Searching for public methods in the implementation..." rg "^[[:space:]]*(?:public|protected).*\(" --type ts --pcre2 packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts || trueLength of output: 508
Script:
#!/bin/bash # View the implementation file content echo "Viewing CollectionFlowManager implementation..." cat packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts # View the test file content for comparison echo -e "\nViewing test file content..." cat packages/common/src/utils/collection-flow-manager/collection-flow-manager.unit.test.tsLength of output: 8591
services/workflows-service/src/collection-flow/collection-flow.service.ts (2)
21-21: LGTM: Import statement aligns with CollectionFlow context integration.The addition of CollectionFlowContextSchema import is consistent with the PR's objective of enhancing type safety and improving state management.
290-298: 🛠️ Refactor suggestionConsider enhancing type safety, error handling, and documentation.
While the method implementation is straightforward, there are several areas for improvement:
- The method selects more fields than necessary (
stateandconfig) when onlycontextis being returned- Missing error handling for potential undefined context
- Lacks JSDoc documentation explaining the method's purpose and usage
- No runtime type validation of the returned context
Consider applying these improvements:
+ /** + * Retrieves the collection flow context for a given workflow runtime + * @param tokenScope - The token scope containing workflow and project information + * @returns The strongly typed collection flow context + * @throws {NotFoundException} When workflow runtime data is not found + */ async getCollectionFlowContext(tokenScope: ITokenScope): Promise<CollectionFlowContextSchema> { const workflowRuntimeData = await this.workflowService.getWorkflowRuntimeDataById( tokenScope.workflowRuntimeDataId, - { select: { context: true, state: true, config: true } }, + { select: { context: true } }, [tokenScope.projectId], ); + if (!workflowRuntimeData.context) { + throw new NotFoundException('Collection flow context not found'); + } + return workflowRuntimeData.context; }Let's verify the usage of this method to ensure proper error handling in consumers:
packages/common/src/utils/collection-flow-manager/helpers/state-helper.ts (1)
75-83: 🛠️ Refactor suggestionAvoid direct state replacement without validation
The
overridemethod replaces the entire state without any validation, which might lead to inconsistencies or invalid states. Consider adding validation to ensure the new state adheres to the expected schema.Would you like assistance in implementing state validation? If so, we can create a method to validate the state against the
CollectionFlowContextSchema.apps/kyb-app/src/pages/CollectionFlow/CollectionFlow.tsx (4)
31-31: Imports are correctly addedThe
CollectionFlowManagerandCollectionFlowStateEnumare appropriately imported from@ballerine/common.
96-99: Conditional rendering for Approved state is correctThe condition correctly checks if the
collectionFlowStateisapprovedand renders the<Approved />component accordingly. The optional chaining and equality check are used appropriately.
101-104: Conditional rendering for Rejected state is correctSimilarly, the condition checks if the
collectionFlowStateisrejectedand renders the<Rejected />component. The use of optional chaining and the equality check are correct.
74-78:⚠️ Potential issueFix syntax error in
initialContextdefinitionThe
useMemohook should return an object. Currently, the spread operator is used without wrapping it inside an object literal or including a return statement, which will cause a syntax error.Apply this diff to fix the syntax error:
const initialContext: CollectionFlowContext = useMemo(() => { + return { ...context, + } as CollectionFlowContext; }, [context]);Likely invalid or redundant comment.
services/workflows-service/src/workflow/workflow.service.ts (2)
1474-1478:⚠️ Potential issuePotential PII Exposure: Storing Customer Details in Metadata
The customer details (
customerId,customerNormalizedName,customerName) are being stored in the workflow context's metadata. Ensure that including this sensitive information is necessary and that appropriate security measures are in place to protect it from unauthorized access or exposure.
1571-1576:⚠️ Potential issueSecurity Consideration: Handling Tokens in Configuration
Updating the
tokenIdincollectionFlowManager's configuration may expose sensitive tokens within the workflow context. Verify that tokens are handled securely and are not inadvertently exposed or logged. Consider encrypting sensitive information and ensuring tokens are stored and transmitted securely.
.../DynamicUI/StateManager/components/ActionsHandler/action-handlers/action-handler.abstract.ts
Show resolved
Hide resolved
packages/common/src/utils/collection-flow-manager/schemas/config-schema.ts
Show resolved
Hide resolved
packages/common/src/utils/collection-flow-manager/schemas/config-schema.ts
Show resolved
Hide resolved
packages/common/src/utils/collection-flow-manager/schemas/config-schema.ts
Outdated
Show resolved
Hide resolved
packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts
Show resolved
Hide resolved
packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts
Show resolved
Hide resolved
packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts
Show resolved
Hide resolved
chesterkmr
force-pushed
the
illiar/feat/app-state
branch
from
b14de72 to
cbd5e63
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
packages/common/src/schemas/documents/collection-flow-context-schema.ts (1)
4-17: Consider standardizing the enum value naming convention.While the states are well-documented, the naming convention could be more consistent:
- 'inprogress' could be 'in_progress' to follow common naming patterns
- 'revision' could be 'in_revision' to match the pattern of 'in_progress'
export enum CollectionFlowStateEnum { pending = 'pending', - inprogress = 'inprogress', + inprogress = 'in_progress', approved = 'approved', rejected = 'rejected', - revision = 'revision', + revision = 'in_revision', failed = 'failed', }services/workflows-service/src/workflow/workflow.controller.internal.ts (1)
Line range hint
291-310: Consider improving error handling and code structureWhile the addition of revision handling is valuable, there are several potential improvements:
- The second service call
updateWorkflowInRevisionshould have its own error handling to ensure failures are properly caught and don't result in inconsistent state.- The variable reassignment pattern could be simplified.
Consider refactoring to:
- let workflowData = await this.service.updateDocumentDecisionById( + const workflowData = await this.service.updateDocumentDecisionById( { workflowId: params?.id, documentId: params?.documentId, documentsUpdateContextMethod: query.contextUpdateMethod, }, { status: data?.decision, reason: data?.reason, comment: data?.comment, }, projectIds, currentProjectId, ); if (data.decision === 'revision') { - workflowData = await this.service.updateWorkflowInRevision(params.id, currentProjectId); + return await this.service.updateWorkflowInRevision(params.id, currentProjectId); } - return workflowData; + return workflowData;Also, consider adding documentation to explain the revision workflow and its implications.
services/workflows-service/src/workflow/workflow.service.ts (1)
2183-2194: Enhance state management with validation and error handling.The state transition logic could be more robust. Consider:
- Adding validation for allowed state transitions
- Implementing error handling for invalid states
- Adding logging for state transitions for better debugging
Example enhancement:
if (context?.collectionFlow) { const collectionFlowManager = new CollectionFlowManager(context); + const previousState = collectionFlowManager.state().collectionFlowState; if (currentState in CollectionFlowStateEnum) { + if (!isValidStateTransition(previousState, currentState)) { + throw new InvalidStateTransitionError(previousState, currentState); + } collectionFlowManager.state().collectionFlowState = currentState; + logger.info('Collection flow state transition', { + from: previousState, + to: currentState + }); } context = collectionFlowManager.context; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- apps/kyb-app/src/helpers/prepareInitialUIState.ts (2 hunks)
- apps/kyb-app/src/pages/CollectionFlow/CollectionFlow.tsx (4 hunks)
- packages/common/src/schemas/documents/collection-flow-context-schema.ts (1 hunks)
- services/workflows-service/package.json (1 hunks)
- services/workflows-service/src/workflow/workflow.controller.internal.ts (3 hunks)
- services/workflows-service/src/workflow/workflow.service.ts (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/kyb-app/src/helpers/prepareInitialUIState.ts
🔇 Additional comments (6)
packages/common/src/schemas/documents/collection-flow-context-schema.ts (3)
1-2: LGTM! Clean and focused imports.The imports are minimal and clearly indicate the dependencies needed for the schema definition.
49-49: LGTM! Clean type export.The type export follows TypeBox best practices and provides a clear type alias for the schema.
19-47: 🛠️ Refactor suggestionConsider adding runtime validation for critical fields.
While the schema structure is good, consider adding runtime validation for critical fields to prevent potential issues:
- Add min/max length constraints for
tokenId- Add pattern validation for progress record keys
- Consider making
collectionFlowStaterequired instead of optional since it's crucial for flow controlexport const collectionFlowContextSchema = Type.Composite([ defaultContextSchema, Type.Object({ collectionFlow: Type.Optional( Type.Object({ config: Type.Optional( Type.Object({ apiUrl: Type.String(), - tokenId: Type.String(), + tokenId: Type.String({ minLength: 1, maxLength: 256 }), }), ), - state: Type.Optional( + state: Type.Object({ Type.Object({ uiState: Type.String(), collectionFlowState: Type.Enum(CollectionFlowStateEnum), progress: Type.Optional( - Type.Record(Type.String(), Type.Object({ isCompleted: Type.Boolean() })), + Type.Record( + Type.String({ pattern: '^[a-zA-Z0-9_-]+$' }), + Type.Object({ isCompleted: Type.Boolean() }) + ), ), }), ), additionalInformation: Type.Optional( Type.Object({ customerCompany: Type.Optional(Type.String()), }), ), }), ), }), ]);The previous review comment about enhancing type safety is still valid.
services/workflows-service/src/workflow/workflow.controller.internal.ts (2)
35-38: LGTM: Clean import additions for enhanced validationThe new imports support the implementation of improved request validation using TypeBox and NestJS decorators.
Line range hint
141-167: LGTM: Robust validation implementationThe enhanced validation setup using TypeBox provides comprehensive request validation for both parameters and body, with well-defined error responses. This improvement helps catch invalid inputs early in the request lifecycle.
services/workflows-service/src/workflow/workflow.service.ts (1)
1470-1486: Consider enhancing type safety and validation for CollectionFlowManager initialization.The initialization looks correct, but consider:
- Adding runtime validation for required configuration properties
- Using TypeScript utility types to ensure all required properties are provided
- Adding error handling for missing UI schema elements
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (4)
packages/common/src/utils/collection-flow-manager/collection-flow-manager.unit.test.ts (3)
32-40: Add edge cases for isStarted() behavior.The current test only covers the positive case when uiState is set. Consider adding tests for:
- When uiState is undefined/null
- When collectionFlow is undefined
- When state is undefined
Example:
test('returns false when uiState is undefined', () => { const manager = new CollectionFlowManager({ collectionFlow: { state: {} } } as DefaultContextSchema); expect(manager.isStarted()).toBe(false); });
42-75: Enhance progress object validation.While the tests verify basic progress structure, consider adding assertions for:
- Step progress object schema completeness
- Handling of multiple steps
- Progress state for optional/conditional steps
Example:
test('handles multiple steps correctly', () => { const manager = new CollectionFlowManager({} as DefaultContextSchema, { steps: [ { stateName: 'step1' }, { stateName: 'step2', optional: true } ] }); const context = manager.start(); expect(context.collectionFlow?.state?.progress).toEqual({ step1: { isCompleted: false }, step2: { isCompleted: false, optional: true } }); });
125-158: Add test coverage for StateHelper methods.While the tests verify state retrieval, they don't cover the actual functionality provided by the StateHelper instance.
Add tests for common StateHelper operations:
- Getting/setting current step
- Updating step progress
- Handling state transitions
- State validation methods
Example:
test('allows state manipulation via helper', () => { const manager = createStartedManager(); const state = manager.state(); state.setCurrentStep('step1'); expect(state.getCurrentStep()).toBe('step1'); state.completeStep('step1'); expect(state.isStepCompleted('step1')).toBe(true); });packages/common/src/utils/collection-flow-manager/helpers/state-helper.ts (1)
20-21: Use a dedicated logging utility instead ofconsole.logTo maintain consistent logging practices and better control over log levels across different environments (development, staging, production), consider replacing
console.logstatements with a dedicated logging library or utility used in your application. This enhances maintainability and provides more flexibility in managing logs.Apply this diff to update the logging statements:
- console.log(`UI State transition from ${this.uiState} to ${uiState}`); + Logger.info(`UI State transition from ${this.uiState} to ${uiState}`); ... - console.log( - `Collection flow state transition from ${this.collectionFlowState} to ${collectionFlowState}`, - ); + Logger.info( + `Collection flow state transition from ${this.collectionFlowState} to ${collectionFlowState}`, + ); ... - console.log(`Step: ${step} isCompleted: ${isCompleted}`); + Logger.info(`Step: ${step} isCompleted: ${isCompleted}`); ... - console.log('StateHelper, context override', this.context.collectionFlow!.state); + Logger.debug('StateHelper, context override', this.context.collectionFlow!.state);Assuming
Loggeris the logging utility in your application.Also applies to: 37-39, 65-65, 76-76
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
- apps/kyb-app/src/domains/collection-flow/types/flow-context.types.ts (1 hunks)
- apps/kyb-app/src/pages/CollectionFlow/CollectionFlow.tsx (4 hunks)
- packages/common/src/consts/index.ts (1 hunks)
- packages/common/src/index.ts (3 hunks)
- packages/common/src/schemas/documents/default-context-schema.ts (2 hunks)
- packages/common/src/schemas/index.ts (2 hunks)
- packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/collection-flow-manager.unit.test.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/helpers/config-helper.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/helpers/state-helper.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/helpers/state-helper.unit.test.ts (1 hunks)
- services/workflows-service/src/collection-flow/collection-flow.service.ts (2 hunks)
- services/workflows-service/src/workflow/workflow.controller.internal.ts (3 hunks)
- services/workflows-service/src/workflow/workflow.service.ts (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/kyb-app/src/domains/collection-flow/types/flow-context.types.ts
- packages/common/src/index.ts
- packages/common/src/schemas/index.ts
- packages/common/src/utils/collection-flow-manager/helpers/config-helper.ts
- packages/common/src/utils/collection-flow-manager/helpers/state-helper.unit.test.ts
- services/workflows-service/src/collection-flow/collection-flow.service.ts
- services/workflows-service/src/workflow/workflow.controller.internal.ts
🔇 Additional comments (15)
packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts (2)
19-21: LGTM! Clean implementation of state check.The method effectively uses optional chaining to safely check the flow state.
62-76: LGTM! Well-structured helper implementation.Good use of helper classes and null checks before returning instances.
packages/common/src/consts/index.ts (1)
191-191: Verify type usage across the codebase.The type definition follows the correct pattern. However, since this is a new type that will be used for state management, we should verify its integration.
Let's check how this type is being used:
✅ Verification successful
Based on the search results, I can now generate the final response:
Type is correctly integrated and used in the codebase
The
TCollectionFlowStatetype is properly used in the collection flow manager's state helper implementation and its tests:
- Used in
packages/common/src/utils/collection-flow-manager/helpers/state-helper.tsfor type safety- Validated against
CollectionFlowStatesenum in the state helper's setter- Test coverage exists in
state-helper.unit.test.tsincluding invalid state handling🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of TCollectionFlowState type rg "TCollectionFlowState" -A 3 # Search for direct usage of CollectionFlowStates values rg -e "pending'|'inprogress'|'completed'" -A 3Length of output: 23562
packages/common/src/utils/collection-flow-manager/collection-flow-manager.unit.test.ts (4)
18-30: LGTM! Good negative test coverage.The test effectively validates the failure case when attempting to start an already started flow.
77-89: Remove redundant test block.This test block duplicates assertions already covered in the "context will be updated on start" describe block.
9-9: 🛠️ Refactor suggestionCreate a test factory for DefaultContextSchema.
Multiple instances of type casting empty objects to
DefaultContextSchemamake the tests brittle and bypass type safety. This pattern appears in several places throughout the test file.Create a test factory function:
function createTestContext(partial?: Partial<DefaultContextSchema>): DefaultContextSchema { return { collectionFlow: { state: {}, ...partial?.collectionFlow } }; } // Usage: const manager = new CollectionFlowManager(createTestContext());Also applies to: 13-13, 64-64
91-123: 🛠️ Refactor suggestionReduce setup duplication in config tests.
The config retrieval tests contain duplicated setup code for creating and starting the CollectionFlowManager.
Create a setup helper:
function createStartedManager(config = {}) { const manager = new CollectionFlowManager(createTestContext(), { apiUrl: 'https://api.example.com', tokenId: '1234567890', steps: [{ stateName: 'step1' }], ...config }); return new CollectionFlowManager(manager.start()); }apps/kyb-app/src/pages/CollectionFlow/CollectionFlow.tsx (4)
11-14: LGTM: New imports enhance error handling and state management capabilities.The added imports properly support the new error handling and state management features.
Also applies to: 34-34
109-113: LGTM: Clean state-based conditional rendering.The implementation properly handles state transitions with type-safe checks.
129-149: Consider cleanup of CollectionFlowManager instance.The CollectionFlowManager instance is created within the onNext callback but there's no cleanup mechanism. While this might not cause immediate issues, it's good practice to clean up resources, especially if they maintain internal state.
Let's check if CollectionFlowManager needs cleanup:
#!/bin/bash # Search for cleanup methods in CollectionFlowManager ast-grep --pattern 'class CollectionFlowManager { $$$ destroy() { $$$ } $$$ }'If CollectionFlowManager has cleanup methods, consider wrapping the instance in a useCallback or moving it outside the callback to prevent unnecessary recreations:
+const handleNext = useCallback(async (tools, prevState, currentState) => { + const collectionFlowManager = new CollectionFlowManager(stateApi.getContext()); // ... rest of the code +}, [stateApi]); -onNext={async (tools, prevState, currentState) => { +onNext={handleNext}
81-91: 🛠️ Refactor suggestionEnhance type safety in initialContext setup.
The current implementation makes assumptions about the context type without validation.
Let's verify the context structure:
Consider adding runtime validation:
const initialContext: CollectionFlowContext = useMemo(() => { + if (!context || !context.collectionFlow) { + throw new Error('Invalid context structure'); + } const collectionFlowManager = new CollectionFlowManager(context as CollectionFlowContext);services/workflows-service/src/workflow/workflow.service.ts (3)
123-124: LGTM! Well-defined constant for collection flow events.The constant clearly defines the allowed collection flow event states, following proper naming conventions.
1525-1526: Previous review comment about scattered context updates still applies.The code continues to show multiple scattered context updates which could lead to inconsistent states.
Also applies to: 1574-1579
1472-1488: Verify the empty token initialization.While the initialization looks good overall, initializing
tokenIdas an empty string might cause issues if the token is required for subsequent operations. Consider adding validation or documentation to clarify when the token gets populated.✅ Verification successful
Let me try a different search strategy to understand the usage of
tokenIdin the codebase.
Empty tokenId initialization is correct and safe
The empty token initialization is intentional and properly handled in the codebase. The search results show that:
- The token is immediately populated after initialization:
collectionFlowManager.config().tokenId = workflowToken.token;
- The
ConfigHelperclass provides proper getter/setter methods fortokenIdwith validation:set tokenId(tokenId: string) { if (!this.context.collectionFlow?.config) { throw new Error('Collection flow config is not set.'); } console.log(`Token ID updated from ${this.tokenId} to ${tokenId}`); this.context.collectionFlow.config.tokenId = tokenId; }
- The token is used in API calls only after it's properly set, as evidenced by the UI definition files where it's used in headers and URLs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for token usage in collection flow manager rg -A 3 "tokenId.*=.*''"Length of output: 32
Script:
#!/bin/bash # Search for CollectionFlowManager class definition and usage of tokenId ast-grep --pattern 'class CollectionFlowManager { $$$ }' # Search for any tokenId references rg -A 3 "tokenId"Length of output: 26265
packages/common/src/schemas/documents/default-context-schema.ts (1)
43-68: LGTM!The addition of the
collectionFlowproperty enhances the context schema effectively. The structure is well-defined, and the use of optional properties allows for flexible state management. The integration ofCollectionFlowStatesensures type safety for the flow state enumeration.
packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts
Show resolved
Hide resolved
packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts
Outdated
Show resolved
Hide resolved
packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts
Show resolved
Hide resolved
packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts
Show resolved
Hide resolved
packages/common/src/utils/collection-flow-manager/helpers/state-helper.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (1)
packages/common/CHANGELOG.md (1)
3-8: Enhance the changelog entry with more details.The changelog entry could be more descriptive to help users understand the impact of these changes. Consider adding:
- A brief description of what the collection flow manager does
- Details about the schema updates
- Any breaking changes or migration steps required
- Links to relevant documentation or examples
This is particularly important as these changes appear to be part of significant state management improvements.
Example of a more detailed entry:
## 0.9.44 ### Patch Changes -Added collection flow manager & updated schema +- Added collection flow manager for improved state management and context handling +- Updated schema to support new state management features +- Breaking changes: + * New required fields in state context + * Modified method signatures for action handlers +- Migration guide: [link to docs]
📜 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 (29)
- apps/backoffice-v2/CHANGELOG.md (1 hunks)
- apps/backoffice-v2/package.json (2 hunks)
- apps/kyb-app/CHANGELOG.md (1 hunks)
- apps/kyb-app/package.json (2 hunks)
- apps/kyb-app/src/components/organisms/UIRenderer/elements/SubmitButton/SubmitButton.tsx (2 hunks)
- examples/headless-example/CHANGELOG.md (1 hunks)
- examples/headless-example/package.json (2 hunks)
- packages/blocks/package.json (1 hunks)
- packages/blocks/rollup.config.ts (5 hunks)
- packages/common/CHANGELOG.md (1 hunks)
- packages/common/package.json (3 hunks)
- packages/common/src/schemas/documents/default-context-schema.ts (2 hunks)
- packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/collection-flow-manager.unit.test.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/helpers/config-helper.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/helpers/config-helper.unit.test.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/schemas/config-schema.ts (1 hunks)
- packages/workflow-core/CHANGELOG.md (1 hunks)
- packages/workflow-core/package.json (2 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/workflows-service/CHANGELOG.md (1 hunks)
- services/workflows-service/package.json (3 hunks)
- services/workflows-service/src/workflow/workflow.service.ts (8 hunks)
- websites/docs/package.json (1 hunks)
✅ Files skipped from review due to trivial changes (12)
- apps/backoffice-v2/CHANGELOG.md
- apps/backoffice-v2/package.json
- apps/kyb-app/CHANGELOG.md
- apps/kyb-app/package.json
- examples/headless-example/CHANGELOG.md
- examples/headless-example/package.json
- packages/workflow-core/CHANGELOG.md
- sdks/web-ui-sdk/CHANGELOG.md
- sdks/workflow-browser-sdk/CHANGELOG.md
- sdks/workflow-node-sdk/CHANGELOG.md
- sdks/workflow-node-sdk/package.json
- services/workflows-service/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/common/package.json
- packages/common/src/schemas/documents/default-context-schema.ts
- packages/common/src/utils/collection-flow-manager/collection-flow-manager.unit.test.ts
- packages/common/src/utils/collection-flow-manager/helpers/config-helper.unit.test.ts
- packages/common/src/utils/collection-flow-manager/schemas/config-schema.ts
🔇 Additional comments (21)
packages/common/src/utils/collection-flow-manager/helpers/config-helper.ts (4)
3-4: Add context validation in constructor.The existing review comment about adding constructor validation is still valid. The constructor should validate that the provided context is not null/undefined to fail fast.
6-8: Improve type safety in getter.The existing review comment about improving getter type safety is still valid. The current implementation uses type assertion which could mask runtime errors.
10-17: Replace console.log with proper logging and improve validation.The existing review comments about replacing console.log with proper logging and improving validation are still valid.
19-27: Improve override method safety and add security validation.The existing review comment about improving override method safety is still valid. Additionally, let's verify if the config contains sensitive data:
#!/bin/bash # Search for sensitive data patterns in config usage rg -i '(password|secret|key|token|credential).*config'If the config contains sensitive data, consider adding validation for required security properties:
interface SecurityConfig { tokenId: string; apiUrl: string; // Add other required security properties } function validateSecurityConfig(config: unknown): config is SecurityConfig { // Add validation logic }sdks/workflow-browser-sdk/package.json (1)
4-4: Consider deferring version bumps until the PR is ready for release.Since this is marked as a WIP PR, consider whether version bumps should be made at this stage or deferred until the changes are finalized.
Let's check if there are any release-related workflows that might be triggered by version changes:
packages/workflow-core/package.json (1)
4-4: LGTM! Version bump and dependency update look appropriate.The version increment and dependency update align with the changes being made across multiple packages in the ecosystem.
Also applies to: 34-34
packages/blocks/package.json (1)
49-49: LGTM! Verify rollup configuration for JSON plugin.The addition of
@rollup/plugin-jsonis appropriate for handling JSON imports.Let's verify the rollup configuration:
✅ Verification successful
JSON plugin is properly configured in rollup
The
@rollup/plugin-jsonis correctly imported and used in all build configurations:
- ESM build (line 156)
- CJS build (line 182)
- UMD development build (line 213)
- UMD production build (line 245)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the rollup config properly includes the JSON plugin # Test: Search for json plugin configuration rg -A 5 "@rollup/plugin-json" "packages/blocks/rollup.config"Length of output: 129
Script:
#!/bin/bash # Let's first find the rollup config file as it might have a different extension fd "rollup.config" packages/blocks # Also search for any file containing rollup configuration fd -e js -e ts -e mjs "rollup" packages/blocksLength of output: 213
Script:
#!/bin/bash # Check both rollup config files for json plugin configuration cat packages/blocks/rollup.config.js packages/blocks/rollup.config.ts # Also specifically search for json plugin imports in these files rg -i "json.*plugin" packages/blocks/rollup.config.js packages/blocks/rollup.config.tsLength of output: 5473
packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts (3)
19-21: LGTM! Clean implementation of state check.The method effectively uses optional chaining for safe property access and provides a clear boolean response.
61-75: LGTM! Well-implemented helper methods with proper error handling.Both methods include appropriate null checks and provide clear error messages before returning helper instances.
34-38:⚠️ Potential issueStrengthen type safety in state initialization.
The type assertion
as stringonstateNamecould lead to runtime errors ifstepsis empty or undefined. Consider adding proper validation and type guards.+ if (!this._config?.steps?.length) { + throw new Error('Collection flow configuration must contain at least one step.'); + } const state: NonNullable<DefaultContextSchema['collectionFlow']>['state'] = { - uiState: this._config?.steps[0]?.stateName as string, + uiState: this._config.steps[0].stateName, collectionFlowState: CollectionFlowStates.pending, progress: this.buildProgressState(), };Likely invalid or redundant comment.
sdks/web-ui-sdk/package.json (2)
99-99: Verify @ballerine/common upgrade impact.Since this PR is marked as WIP, please ensure that the upgrade to @ballerine/[email protected] doesn't introduce any breaking changes that could affect the web-ui-sdk.
Let's check for potential breaking changes:
#!/bin/bash # Description: Look for breaking changes indicators # Expected: No major version changes or breaking change indicators # Check for BREAKING CHANGE markers in recent commits git log --grep="BREAKING CHANGE" -i --since="1 week ago" # Look for major version bumps in @ballerine/common rg '"@ballerine/common":\s*"[^"]*"' -g 'package.json'
24-24: LGTM on version bump.The patch version increment follows semantic versioning principles.
Let's verify the version bump aligns with other package updates:
✅ Verification successful
Version bump to 1.5.45 is appropriate and follows the project's versioning pattern
The version increment from 1.5.44 to 1.5.45 in web-ui-sdk is consistent with:
- The package's independent versioning scheme (1.x.x)
- The nature of the changes (patch update)
- The corresponding update of @ballerine/common dependency from 0.9.43 to 0.9.44
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check version patterns across related packages # Expected: Consistent version updates across related packages # Search for version fields in package.json files rg '"version":\s*"[^"]*"' -g 'package.json'Length of output: 861
Script:
#!/bin/bash # Let's check the version fields to verify alignment rg '"version":\s*"[^"]*"' -g 'package.json'Length of output: 1481
packages/blocks/rollup.config.ts (2)
16-16: LGTM: JSON plugin import added correctly.The @rollup/plugin-json import is properly placed with other plugin imports.
111-111: LGTM: JSON plugin consistently integrated across all build formats.The json() plugin is correctly added to all relevant build configurations (ESM, CJS, UMD dev, and UMD prod), ensuring consistent JSON handling across different module formats.
Also applies to: 134-134, 157-157, 186-186
services/workflows-service/package.json (1)
4-4: LGTM: Coordinated version updates across @ballerine packages.The version bump and dependency updates are consistently applied across related @ballerine packages, maintaining version alignment within the ecosystem.
Also applies to: 50-52
services/workflows-service/src/workflow/workflow.service.ts (4)
12-14: LGTM: New imports for OCR functionalityThe imports are properly organized and necessary for the new OCR functionality.
123-124: LGTM: Well-defined whitelist for collection flow eventsThe constant clearly defines the allowed states for collection flow transitions.
1524-1525: Previous review comment about context updates is still applicable
1573-1577: Previous review comment about context updates is still applicableapps/kyb-app/src/components/organisms/UIRenderer/elements/SubmitButton/SubmitButton.tsx (2)
15-15: Approved: Importing CollectionFlowManager and CollectionFlowStatesThe import statement correctly brings in
CollectionFlowManagerandCollectionFlowStatesfrom@ballerine/common, which are utilized in the updated state management logic.
78-78: Verify that updated context is properly synchronizedAfter setting the new state, the updated context is set with
stateApi.setContext(collectionFlowManager.context);. Ensure that this action correctly synchronizes the context across all relevant components and that there are no stale references to the old context.To confirm that the context is being updated as expected throughout the application, consider checking dependent components or running tests that validate context-dependent functionality.
packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts
Show resolved
Hide resolved
packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts
Show resolved
Hide resolved
apps/kyb-app/src/components/organisms/UIRenderer/elements/SubmitButton/SubmitButton.tsx
Show resolved
Hide resolved
chesterkmr
force-pushed
the
illiar/feat/app-state
branch
from
cb12d31 to
b2fc722
Compare
…cker in backoffice
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (8)
packages/common/src/utils/collection-flow-manager/helpers/config-helper.ts (3)
24-26: Add JSDoc comment explaining steps immutability.
The error message could be more informative about why steps can't be modified after initialization.
+/**
+ * Steps are immutable after initialization to ensure flow consistency.
+ * Modifications to steps during runtime could lead to unexpected behavior.
+ * @throws {Error} Always throws to prevent steps modification
+ */
set steps(_) {
throw new Error('Setting steps is not allowed after initialization.');
}28-36: Add return type annotation for override method.
The method should explicitly declare its return type for better type safety.
- override(config: NonNullable<NonNullable<DefaultContextSchema['collectionFlow']>['config']>) {
+ override(
+ config: NonNullable<NonNullable<DefaultContextSchema['collectionFlow']>['config']>,
+ ): DefaultContextSchema {5-5: Add readonly modifier to context parameter.
The context parameter should be marked as readonly to prevent accidental modifications.
- constructor(private context: DefaultContextSchema) {}
+ constructor(private readonly context: DefaultContextSchema) {}apps/kyb-app/src/components/organisms/DynamicUI/Page/hooks/usePageErrors/usePageErrors.ts (3)
4-4: LGTM: Type safety improvement for context parameter.
The change from AnyObject to CollectionFlowContext improves type safety. However, consider extending this improvement to the helper functions.
Consider updating the helper functions to use CollectionFlowContext:
-export const selectDirectors = (context: AnyObject) =>
+export const selectDirectors = (context: CollectionFlowContext) =>
(context?.entity?.data?.additionalInfo?.directors as AnyObject[]) || [];
-export const selectDirectorsDocuments = (context: unknown): Document[] =>
+export const selectDirectorsDocuments = (context: CollectionFlowContext): Document[] =>Also applies to: 26-26
Line range hint 47-52: Remove @ts-ignore by improving type definitions.
Multiple ts-ignore comments suggest underlying type safety issues that should be addressed.
Consider creating proper type definitions for the document decision:
interface DocumentDecision {
status: 'revision' | 'rejected' | 'approved';
revisionReason?: string;
}
interface DocumentWithDecision extends Document {
decision?: DocumentDecision;
}Then update the code:
- //@ts-ignore
- !(document?.decision?.status == 'revision' || document?.decision?.status == 'rejected')
+ !(document as DocumentWithDecision)?.decision?.status === 'revision' ||
+ !(document as DocumentWithDecision)?.decision?.status === 'rejected'Line range hint 65-72: Define error field types as constants.
The error type 'warning' is hardcoded and document ID casting could be improved.
Consider defining an enum or constant for error types and improving ID typing:
enum ErrorFieldType {
WARNING = 'warning',
ERROR = 'error'
}
interface DocumentId {
id: string;
}
// Then update the code:
return {
fieldId: `document-error-${(document as DocumentId).id}`,
fieldDestination: `document-error-${(document as DocumentId).id}`,
message,
type: ErrorFieldType.WARNING,
};services/workflows-service/scripts/workflows/runtime/generate-initial-collection-flow-example.ts (1)
87-87: Consider making the workflow version configurable
The workflow version is hardcoded to 1, which might cause issues with version management.
Consider making it configurable:
- workflowDefinitionVersion: 1,
+ workflowDefinitionVersion: uiDefinition.version ?? 1,packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts (1)
28-31: Extract default configuration to constants.
Default values are hardcoded. Consider moving them to named constants for better maintainability and reusability.
+ const DEFAULT_CONFIG = {
+ apiUrl: '',
+ steps: [],
+ } as const;
const config: NonNullable<DefaultContextSchema['collectionFlow']>['config'] = {
- apiUrl: this._config?.apiUrl || '',
- steps: this._config?.steps || [],
+ apiUrl: this._config?.apiUrl || DEFAULT_CONFIG.apiUrl,
+ steps: this._config?.steps || DEFAULT_CONFIG.steps,
};📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
- apps/backoffice-v2/src/common/components/molecules/ProcessTracker/hooks/useProcessTracker/process-tracker-adapters/collection-flow.process-tracker.ts (1 hunks)
- apps/backoffice-v2/src/domains/workflows/fetchers.ts (2 hunks)
- apps/kyb-app/src/components/layouts/AppShell/Navigation.tsx (1 hunks)
- apps/kyb-app/src/components/organisms/DynamicUI/Page/hooks/usePageErrors/usePageErrors.ts (2 hunks)
- apps/kyb-app/src/pages/CollectionFlow/CollectionFlow.tsx (4 hunks)
- packages/common/src/schemas/documents/default-context-schema.ts (2 hunks)
- packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/helpers/config-helper.ts (1 hunks)
- packages/common/src/utils/collection-flow-manager/schemas/config-schema.ts (1 hunks)
- services/workflows-service/package.json (2 hunks)
- services/workflows-service/prisma/data-migrations (1 hunks)
- services/workflows-service/scripts/workflows/runtime/generate-initial-collection-flow-example.ts (2 hunks)
- services/workflows-service/src/collection-flow/helpers/get-steps-in-order.ts (1 hunks)
- services/workflows-service/src/workflow/workflow.service.ts (9 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/common/src/schemas/documents/default-context-schema.ts
- packages/common/src/utils/collection-flow-manager/schemas/config-schema.ts
- services/workflows-service/package.json
- services/workflows-service/prisma/data-migrations
🔇 Additional comments (20)
services/workflows-service/src/collection-flow/helpers/get-steps-in-order.ts (2)
27-27: LGTM!
The transformation logic is clean and uses appropriate 1-based indexing for order numbers.
14-20:
Address potential issues in state machine initialization.
Several concerns in the state machine initialization:
- Empty runtime ID could cause issues in tracking or debugging
- Using
anytype for definition bypasses type safety - Purpose of empty extensions and workflowContext is unclear
Consider this safer implementation:
const stateMachine = createWorkflow({
- runtimeId: '',
+ runtimeId: `steps-order-${Date.now()}`,
definition: (definition as Prisma.JsonObject).definition as any,
definitionType: 'statechart-json',
- extensions: {},
- workflowContext: {},
+ extensions: undefined,
+ workflowContext: undefined,
});apps/backoffice-v2/src/common/components/molecules/ProcessTracker/hooks/useProcessTracker/process-tracker-adapters/collection-flow.process-tracker.ts (1)
50-50: Previous review comment is still applicable.
The suggestion to add an explicit step existence check from the previous review remains valid for this code.
apps/kyb-app/src/components/layouts/AppShell/Navigation.tsx (3)
15-15: LGTM: Clean hook usage
The addition of payload from useStateManagerContext aligns with the broader context management changes.
24-24: Verify edge case handling in isFirstStep determination
The isFirstStep determination now depends on currentPageNumber calculation. While simpler, ensure this handles all edge cases correctly, especially when:
- The flow configuration is loading
- The step is not found in the configuration
- The current page state is transitioning
Let's check for potential edge cases in the codebase:
#!/bin/bash
# Look for other usages of isFirstStep to ensure consistent handling
rg "isFirstStep" --type ts -C 3
# Check for similar navigation conditions that might need alignment
ast-grep --pattern 'if ($_ === 1) { $$$ }'20-22:
Add fallback handling for step resolution
The current implementation might return undefined if the step is not found, which could affect the isFirstStep check. Consider adding explicit fallback handling.
Here's a suggested improvement:
- const currentPageNumber = payload?.collectionFlow?.config?.steps?.find(
- step => step.stateName === currentPage?.stateName,
- )?.orderNumber;
+ const currentPageNumber = payload?.collectionFlow?.config?.steps?.find(
+ step => step.stateName === currentPage?.stateName,
+ )?.orderNumber ?? 1; // Explicit fallback to first stepLet's verify if there are other components relying on this step resolution logic:
services/workflows-service/scripts/workflows/runtime/generate-initial-collection-flow-example.ts (2)
1-3: LGTM: Imports are appropriate for the new functionality
The imports correctly bring in the necessary components for state management and workflow creation.
48-66: Skip: Previous review comment about type safety and hardcoded values is still valid
packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts (4)
19-21: LGTM! Clean and safe implementation.
The method effectively uses optional chaining to safely check the nested state.
62-76: LGTM! Well-structured helper initialization.
Good error handling and separation of concerns using helper classes.
35-39:
Improve type safety in state initialization.
The type assertion on stateName could mask potential issues. Consider adding proper validation.
+ if (!this._config?.steps?.length) {
+ throw new Error('Steps configuration is required to start the collection flow.');
+ }
const state: NonNullable<DefaultContextSchema['collectionFlow']>['state'] = {
- uiState: this._config?.steps[0]?.stateName as string,
+ uiState: this._config.steps[0].stateName,
collectionFlowState: CollectionFlowStates.pending,
progress: this.buildProgressState(),
};Likely invalid or redundant comment.
11-17:
Consider making the configuration parameter required.
The _config parameter is marked as optional, but it's essential for the class's core functionality (used in start(), buildProgressState()). Making it required would prevent runtime errors and improve type safety.
- constructor(public context: TContext, private readonly _config?: CollectionFlowManagerConfig) {
+ constructor(public context: TContext, private readonly _config: CollectionFlowManagerConfig) {
- if (_config && !collectionFlowConfigValidationSchema(_config)) {
+ if (!collectionFlowConfigValidationSchema(_config)) {
throw new Error('Invalid collection flow manager config.');
}
this.context = structuredClone(context);
}Likely invalid or redundant comment.
apps/backoffice-v2/src/domains/workflows/fetchers.ts (2)
161-161: LGTM: Appropriate omission of collectionFlow in child workflows.
The omission of collectionFlow from child workflow contexts is correct as it prevents unnecessary nesting and potential circular references.
118-137: Verify schema compatibility with existing state management.
Let's verify that the new collection flow schema is compatible with existing state management components.
✅ Verification successful
Let me analyze the results and gather more specific information about the schema usage.
The initial results show widespread usage across the codebase, but we need to verify the actual implementations to ensure schema compatibility. This will help us understand if the schema changes align with the existing usage patterns.
Based on the comprehensive results, I can now generate the final response:
Schema changes are compatible with existing state management
The new collection flow schema in fetchers.ts aligns perfectly with the existing implementation:
- The enum values (
pending,inprogress,completed, etc.) match all usage across the codebase - The state structure (
uiState,collectionFlowState,progress) is consistent with the CollectionFlowManager implementation - The schema is used consistently in components like:
CollectionFlow.tsxfor state transitionsSubmitButton.tsxfor completion handlingStateManager.tsxfor UI state management- Process tracking components for progress visualization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential incompatibilities with the new collection flow schema
# Test 1: Search for direct access to old flowConfig structure
echo "Checking for old flowConfig usage..."
rg -l "flowConfig" --type ts
# Test 2: Search for components that might need updates for the new schema
echo "Checking components using collection flow state..."
rg -l "collectionFlowState|uiState|progress" --type ts
# Test 3: Check for any hardcoded state values that might not match the enum
echo "Checking for hardcoded state values..."
rg -l "'pending'|'inprogress'|'completed'|'approved'|'revision'|'rejected'|'failed'" --type ts
Length of output: 10113
Script:
#!/bin/bash
# Let's check the actual implementation details of collection flow state usage
# Check the specific schema implementations
echo "Checking schema implementations..."
rg -A 10 "collectionFlow.*state" --type ts
# Check for any type definitions using these states
echo "Checking type definitions..."
ast-grep --pattern 'type $_ = {
$$$
collectionFlowState
$$$
}'
# Check for direct state assignments
echo "Checking state assignments..."
rg -A 5 "collectionFlowState.*=" --type ts
Length of output: 44947
apps/kyb-app/src/pages/CollectionFlow/CollectionFlow.tsx (3)
60-62: Skip comment - covered by past review
The previous review comment about improving type safety and readability of getRevisionStateName is still valid.
94-95: Skip comment - covered by past review
The previous review comment about simplifying context usage in prepareInitialUIState is still valid.
132-149: 🛠️ Refactor suggestion
Consider potential race condition in state updates
The state updates are performed sequentially but might be affected by React's state batching. Consider using a single atomic update.
-collectionFlowManager.state().setStepCompletionState(prevState, true);
-collectionFlowManager.state().uiState = currentState;
-
-if (!isAnyStepCompleted) {
- collectionFlowManager.state().collectionFlowState =
- CollectionFlowStates.inprogress;
-}
+// Batch state updates
+const state = collectionFlowManager.state();
+state.setStepCompletionState(prevState, true);
+state.uiState = currentState;
+
+if (!isAnyStepCompleted) {
+ state.collectionFlowState = CollectionFlowStates.inprogress;
+}services/workflows-service/src/workflow/workflow.service.ts (3)
125-126: LGTM! Well-defined constant for workflow states.
The constant is appropriately named and contains the correct set of workflow states.
1524-1525: Previous review comment about scattered context updates is still valid.
The code still has multiple scattered context updates which could lead to inconsistent states.
Please refer to the previous review comment about centralizing context updates and implementing a transaction-like pattern.
Also applies to: 1573-1577, 1583-1590
2183-2206: Previous review comment about refactoring state handling is still valid.
The collection flow state handling logic is complex and could benefit from being refactored into separate methods.
Please refer to the previous review comment about refactoring the state handling logic to improve maintainability.
services/workflows-service/src/collection-flow/helpers/get-steps-in-order.ts
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/helpers/get-steps-in-order.ts
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/helpers/get-steps-in-order.ts
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/helpers/get-steps-in-order.ts
Show resolved
Hide resolved
packages/common/src/utils/collection-flow-manager/helpers/config-helper.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/backoffice-v2/src/domains/workflows/fetchers.ts (1)
138-138: Consider adding validation for additionalInformation
The additionalInformation field could benefit from more specific validation to ensure data integrity.
Consider adding validation for known fields or at least ensuring non-empty keys:
- additionalInformation: z.record(z.string(), z.unknown()).optional(),
+ additionalInformation: z.record(
+ z.string().min(1, 'Key must not be empty'),
+ z.unknown()
+ ).optional(),📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/backoffice-v2/src/domains/workflows/fetchers.ts (3 hunks)
- apps/kyb-app/src/components/organisms/DynamicUI/StateManager/hooks/useMachineLogic/useMachineLogic.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/kyb-app/src/components/organisms/DynamicUI/StateManager/hooks/useMachineLogic/useMachineLogic.ts
🔇 Additional comments (3)
apps/backoffice-v2/src/domains/workflows/fetchers.ts (3)
9-9: LGTM: Import of CollectionFlowStates
The import is correctly placed and appropriately used in the schema definition.
162-162: LGTM: Child workflow context update
The omission of collectionFlow from child workflows is correctly implemented and maintains schema consistency.
119-138: Verify migration path for existing workflows
The introduction of the new collectionFlow schema structure might affect existing workflows. Please ensure there's a migration strategy in place.
Let's check for existing workflow data structure:
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- services/workflows-service/src/workflow/workflow.service.ts (7 hunks)
🔇 Additional comments (2)
services/workflows-service/src/workflow/workflow.service.ts (2)
1552-1567: LGTM: Collection flow initialization is well structured.
The initialization properly sets up the CollectionFlowManager with required configuration and starts the flow.
2173-2196: 🛠️ Refactor suggestion
Simplify collection flow state transition logic.
The current state transition logic is complex and could be simplified for better readability and maintainability.
- const nextCollectionFlowState = COLLECTION_FLOW_EVENTS_WHITELIST.includes(type)
- ? type
- : // Using current state of workflow for approved, rejected, failed
- COLLECTION_FLOW_EVENTS_WHITELIST.includes(currentState)
- ? currentState
- : undefined;
+ const nextCollectionFlowState = determineNextCollectionFlowState(type, currentState);
+ private determineNextCollectionFlowState(type: CollectionFlowEvent, currentState: string): CollectionFlowEvent | undefined {
+ if (COLLECTION_FLOW_EVENTS_WHITELIST.includes(type)) {
+ return type;
+ }
+
+ return COLLECTION_FLOW_EVENTS_WHITELIST.includes(currentState) ? currentState : undefined;
+ }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
services/workflows-service/scripts/workflows/runtime/generate-initial-collection-flow-example.ts (1)
Line range hint 97-105: Consider making token expiration configurable
The token expiration is hardcoded to 30 days. Consider making this configurable through environment variables or configuration settings to allow for different expiration periods based on deployment requirements.
+const TOKEN_EXPIRATION_DAYS = process.env.TOKEN_EXPIRATION_DAYS || 30;
const workflowToken = await prismaClient.workflowRuntimeDataToken.create({
data: {
endUserId: endUserId,
token: token,
workflowRuntimeDataId: workflowRuntime.id,
projectId: projectId,
- expiresAt: new Date(Date.now() + 30 * 24 * 60 * 60 * 1000),
+ expiresAt: new Date(Date.now() + TOKEN_EXPIRATION_DAYS * 24 * 60 * 60 * 1000),
},
});📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- services/workflows-service/scripts/workflows/runtime/generate-initial-collection-flow-example.ts (2 hunks)
🔇 Additional comments (3)
services/workflows-service/scripts/workflows/runtime/generate-initial-collection-flow-example.ts (3)
6-30: Previous review comment about type safety and error handling is still applicable
The concerns about type safety, error handling, and infinite loop risk raised in the previous review have not been addressed.
48-69: Previous review comment about type safety and hardcoded values is still applicable
The concerns about type definitions and hardcoded values in the initialContext object are still valid.
71-83: Previous review comment about error handling and validation is still applicable
The concerns about proper error handling and validation for workflow initialization are still valid.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (6)
packages/common/src/utils/collection-flow-manager/helpers/state-helper.ts (1)
4-82: Consider architectural improvements for better maintainabilityThe StateHelper class could benefit from several architectural improvements:
- Implement dependency injection for logging
- Use observer pattern for state changes to decouple state updates from logging
- Split into smaller, focused classes (e.g., separate UI state, flow state, and progress management)
Example implementation of observer pattern:
interface StateChangeObserver { onStateChange(change: { type: string; from: unknown; to: unknown }): void; } class StateHelper { private observers: StateChangeObserver[] = []; addObserver(observer: StateChangeObserver) { this.observers.push(observer); } private notifyStateChange(type: string, from: unknown, to: unknown) { this.observers.forEach(observer => observer.onStateChange({ type, from, to })); } }packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts (5)
19-21: Add JSDoc documentation and explicit return type.Consider adding documentation and type annotations for better maintainability and type safety.
+ /** + * Checks if the collection flow has been initialized. + * @returns {boolean} True if the flow has started, false otherwise. + */ - isStarted() { + isStarted(): boolean { return Boolean(this.context?.collectionFlow?.state?.uiState); }
23-54: Enhance method documentation and type safety.The initialization method could benefit from better documentation and type safety.
+ /** + * Initializes the collection flow context with config, state, and additional information. + * @throws {Error} If the flow has already been started + * @returns {TContext} The initialized context + */ - initializeCollectionFlowContext() { + initializeCollectionFlowContext(): TContext {Also, consider validating the initialized context structure before returning:
private validateInitializedContext(context: TContext): void { if (!context.collectionFlow?.config || !context.collectionFlow.state) { throw new Error('Context initialization failed: invalid structure'); } }
62-76: Document helper class capabilities and return types.Add JSDoc documentation to clarify the purpose and capabilities of the helper classes.
+ /** + * Provides access to configuration management capabilities. + * @throws {Error} If the collection flow configuration is not set + * @returns {ConfigHelper} A helper instance for managing configuration + */ - config() { + config(): ConfigHelper { + /** + * Provides access to state management capabilities. + * @throws {Error} If the collection flow state is not set + * @returns {StateHelper} A helper instance for managing state + */ - state() { + state(): StateHelper {
88-98: Document progress state structure and return type.Add documentation to clarify the structure and purpose of the progress state.
+ /** + * Builds the initial progress state for all steps in the configuration. + * Each step is marked as incomplete by default. + * @private + * @returns {Record<string, { isCompleted: boolean }>} A map of step names to their completion status + */ - private buildProgressState() { + private buildProgressState(): Record<string, { isCompleted: boolean }> {
10-99: Consider splitting context management and mutations.The
CollectionFlowManagerclass currently handles both context storage and mutations. Consider splitting these responsibilities:
- Create a separate
CollectionFlowContextclass to manage the context state- Move mutation methods to a separate
CollectionFlowMutatorclassThis separation would:
- Make the code more maintainable
- Make it easier to test
- Allow for better control over state mutations
- Enable implementation of different mutation strategies if needed
Example structure:
class CollectionFlowContext<T extends DefaultContextSchema> { constructor(private context: T) {} getState(): StateHelper { /* ... */ } getConfig(): ConfigHelper { /* ... */ } } class CollectionFlowMutator<T extends DefaultContextSchema> { constructor(private context: CollectionFlowContext<T>) {} initializeFlow(): void { /* ... */ } updateAdditionalInformation(): void { /* ... */ } } // Usage const context = new CollectionFlowContext(initialContext); const mutator = new CollectionFlowMutator(context);
📜 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 (12)
apps/backoffice-v2/package.json(2 hunks)apps/backoffice-v2/src/domains/workflows/fetchers.ts(3 hunks)apps/kyb-app/src/components/organisms/DynamicUI/Page/hooks/usePageErrors/usePageErrors.ts(2 hunks)apps/kyb-app/src/domains/collection-flow/collection-flow.api.ts(2 hunks)apps/kyb-app/src/helpers/prepareInitialUIState.ts(2 hunks)packages/common/src/utils/collection-flow-manager/collection-flow-manager.ts(1 hunks)packages/common/src/utils/collection-flow-manager/collection-flow-manager.unit.test.ts(1 hunks)packages/common/src/utils/collection-flow-manager/helpers/config-helper.ts(1 hunks)packages/common/src/utils/collection-flow-manager/helpers/state-helper.ts(1 hunks)services/workflows-service/prisma/data-migrations(1 hunks)services/workflows-service/scripts/workflows/runtime/generate-initial-collection-flow-example.ts(2 hunks)services/workflows-service/src/workflow/workflow.service.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/backoffice-v2/package.json
- apps/kyb-app/src/components/organisms/DynamicUI/Page/hooks/usePageErrors/usePageErrors.ts
- apps/kyb-app/src/domains/collection-flow/collection-flow.api.ts
- packages/common/src/utils/collection-flow-manager/collection-flow-manager.unit.test.ts
- packages/common/src/utils/collection-flow-manager/helpers/config-helper.ts
- services/workflows-service/prisma/data-migrations
🔇 Additional comments (9)
apps/kyb-app/src/helpers/prepareInitialUIState.ts (1)
4-4: LGTM!
The import is correctly placed and properly utilized in the code.
packages/common/src/utils/collection-flow-manager/helpers/state-helper.ts (1)
69-73: Well implemented null-safe step completion check
Good use of optional chaining and nullish coalescing operator for safe property access.
services/workflows-service/scripts/workflows/runtime/generate-initial-collection-flow-example.ts (3)
6-30: Previous review comment about type safety and error handling is still applicable
The existing review comment correctly identified issues with type assertions, error handling, and infinite loop risk in the getStepsInOrder function.
48-69: Previous review comment about type safety and hardcoded values is still applicable
The existing review comment correctly identified issues with type safety and hardcoded values in the initialContext object.
90-90: Verify context structure before database creation
The context is assigned directly from collectionFlowManager without validation.
Let's verify the context structure in the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response:
Context initialization is properly handled before database creation
The code shows that initializeCollectionFlowContext() is called before using the context, and the context follows the DefaultContextSchema type from @ballerine/common. The context is consistently handled across the codebase:
- Context is initialized via
initializeCollectionFlowContext()before being used - The workflow service properly spreads and merges the context with metadata
- The collection flow service has proper type safety through
DefaultContextSchema
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for context structure definitions and validations
rg -A 10 "interface CollectionFlowContext" packages/common/src/
rg -A 10 "type CollectionFlowContext" packages/common/src/
Length of output: 126
Script:
#!/bin/bash
# Let's try searching in the services directory and with a broader pattern
rg -A 10 "CollectionFlowContext" services/
# Also search for the manager class to understand context usage
rg -A 5 "class CollectionFlowManager" services/
# Look for context validation patterns
rg "validateContext|validateFlowContext" services/
Length of output: 5174
Script:
#!/bin/bash
# Let's look at the CollectionFlowManager implementation to understand context initialization
rg -A 15 "initializeCollectionFlowContext" services/
# Also search for the context property definition in the manager
ast-grep --pattern 'class $_Manager {
$$$
context
$$$
}'
Length of output: 3832
Script:
#!/bin/bash
# Let's look for the CollectionFlowManager class implementation
rg -A 20 "class CollectionFlowManager" services/
# And search for any type definitions or interfaces related to the context
rg -A 10 "type.*Context|interface.*Context" services/
Length of output: 13012
apps/backoffice-v2/src/domains/workflows/fetchers.ts (2)
9-9: LGTM! Good use of shared enum for type safety.
The import of CollectionFlowStates from the common package ensures consistent state values across the application.
154-154: LGTM! Appropriate omission of collectionFlow from child workflows.
The omission of collectionFlow from child workflows' context is consistent with the schema changes and maintains a clean hierarchy.
services/workflows-service/src/workflow/workflow.service.ts (2)
125-131: Well-structured type definition and constant declaration!
The implementation follows TypeScript best practices by:
- Using a union type for compile-time safety
- Making the array readonly to prevent modifications
- Using const assertion for better type inference
2179-2202: 🛠️ Refactor suggestion
Refactor state management for better maintainability.
The collection flow state management logic is complex and lacks proper error handling and state transition validation.
Consider extracting the state management logic into a dedicated method:
+ private async updateCollectionFlowState(
+ context: any,
+ currentState: string,
+ eventType: string,
+ ): Promise<any> {
+ try {
+ const nextCollectionFlowState = COLLECTION_FLOW_EVENTS_WHITELIST.includes(eventType)
+ ? eventType
+ : COLLECTION_FLOW_EVENTS_WHITELIST.includes(currentState)
+ ? currentState
+ : undefined;
+
+ this.logger.log('Next collection flow state', {
+ nextCollectionFlowState: nextCollectionFlowState || 'N/A',
+ });
+
+ if (!nextCollectionFlowState) return context;
+
+ const collectionFlowManager = new CollectionFlowManager(context);
+
+ if (currentState in CollectionFlowStates) {
+ // Validate state transition
+ if (!this.isValidStateTransition(currentState, nextCollectionFlowState)) {
+ throw new Error(`Invalid state transition from ${currentState} to ${nextCollectionFlowState}`);
+ }
+ collectionFlowManager.state().collectionFlowState = currentState;
+ }
+
+ return collectionFlowManager.context;
+ } catch (error) {
+ this.logger.error('Failed to update collection flow state', {
+ error,
+ currentState,
+ eventType,
+ });
+ throw error;
+ }
+ }
- let context = snapshot.machine?.context;
-
- // Checking if event type is candidate for "revision" state
- const nextCollectionFlowState = COLLECTION_FLOW_EVENTS_WHITELIST.includes(type)
- ? type
- : // Using current state of workflow for approved, rejected, failed
- COLLECTION_FLOW_EVENTS_WHITELIST.includes(currentState)
- ? currentState
- : undefined;
-
- this.logger.log('Next collection flow state', {
- nextCollectionFlowState: nextCollectionFlowState || 'N/A',
- });
-
- if (nextCollectionFlowState) {
- const collectionFlowManager = new CollectionFlowManager(context);
-
- if (currentState in CollectionFlowStates) {
- collectionFlowManager.state().collectionFlowState = currentState;
- }
-
- context = collectionFlowManager.context;
- }
+ const context = await this.updateCollectionFlowState(
+ snapshot.machine?.context,
+ currentState,
+ type,
+ );Likely invalid or redundant comment.
| const flow = new CollectionFlowManager(context); | ||
|
|
||
| const result = flow.state().isStepCompleted(page.stateName!); | ||
|
|
||
| if (!page.stateName) return false; | ||
|
|
||
| return result; |
There was a problem hiding this comment.
Fix critical logical and safety issues in isPageCompleted.
There are several issues with the current implementation:
- The null check is performed after using
page.stateNamewith non-null assertion - The function could return an incorrect result if
stateNameis null butresultis truthy - Creating a new
CollectionFlowManagerinstance for each call is inefficient
Apply this diff to fix the issues:
-export const isPageCompleted = (page: UIPage, context: CollectionFlowContext) => {
- const flow = new CollectionFlowManager(context);
-
- const result = flow.state().isStepCompleted(page.stateName!);
-
- if (!page.stateName) return false;
-
- return result;
+export const isPageCompleted = (
+ page: UIPage,
+ context: CollectionFlowContext,
+ flow?: CollectionFlowManager
+) => {
+ if (!page.stateName) return false;
+
+ const flowManager = flow ?? new CollectionFlowManager(context);
+ return flowManager.state().isStepCompleted(page.stateName);
};📝 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 flow = new CollectionFlowManager(context); | |
| const result = flow.state().isStepCompleted(page.stateName!); | |
| if (!page.stateName) return false; | |
| return result; | |
| export const isPageCompleted = ( | |
| page: UIPage, | |
| context: CollectionFlowContext, | |
| flow?: CollectionFlowManager | |
| ) => { | |
| if (!page.stateName) return false; | |
| const flowManager = flow ?? new CollectionFlowManager(context); | |
| return flowManager.state().isStepCompleted(page.stateName); | |
| }; |
| @@ -16,7 +23,8 @@ export const prepareInitialUIState = ( | |||
| isRevision, | |||
| elements: {}, | |||
| }; | |||
| if (pages[0]?.stateName === context.state) return initialUIState; | |||
|
|
|||
| if (pages[0]?.stateName === context.collectionFlow?.state?.uiState) return initialUIState; | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the implementation and usage context, I can now provide the final response:
Early return is incorrect and should be removed
The early return condition is problematic because:
- It returns an empty
elementsobject when the first page's state matches the UI state - This skips the initialization of page states in the
elementsobject, which is required for proper state tracking - The function is used in CollectionFlow.tsx to initialize states for all pages, and this early return breaks that requirement
The correct behavior should be to always initialize states for all pages, regardless of the current UI state. Remove the early return condition:
if (pages[0]?.stateName === context.collectionFlow?.state?.uiState) return initialUIState;🔗 Analysis chain
Verify the early return condition.
The early return when pages[0]?.stateName === context.collectionFlow?.state?.uiState could skip initialization of other pages' states. Please verify if this is the intended behavior.
Let's analyze the usage of this condition:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for similar patterns of early returns based on UI state
# to understand if this is a common pattern or potentially incorrect.
echo "Searching for similar UI state checks..."
rg -A 3 "collectionFlow\?\.state\?\.uiState"
echo "Searching for tests that verify this behavior..."
rg -l "prepareInitialUIState.*test"
Length of output: 1494
Script:
#!/bin/bash
# Let's analyze the prepareInitialUIState function implementation and its usage
# First, get the full implementation of prepareInitialUIState
echo "=== prepareInitialUIState implementation ==="
rg -A 20 "^export.*prepareInitialUIState" apps/kyb-app/src/helpers/prepareInitialUIState.ts
# Check where this function is being used
echo -e "\n=== Usage of prepareInitialUIState ==="
rg "prepareInitialUIState"
# Look for state management related tests
echo -e "\n=== State management related tests ==="
fd "state.*test" -t f
Length of output: 1527
| console.log(`UI State transition from ${this.uiState} to ${uiState}`); | ||
| this.context.collectionFlow!.state!.uiState = uiState; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace console.log with proper logging utility
Direct console.log usage should be replaced with a proper logging utility that supports different log levels and structured logging.
Consider implementing a logging service:
interface ILogger {
debug(message: string, context?: Record<string, unknown>): void;
info(message: string, context?: Record<string, unknown>): void;
// ... other levels
}
// Then use it like:
this.logger.info('UI State transition', {
from: this.uiState,
to: uiState
});| constructor(private context: DefaultContextSchema) { | ||
| if (!this.context.collectionFlow?.state) { | ||
| throw new Error('Collection flow state is not set.'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance constructor validation with more descriptive error handling
While the constructor validates state existence, it could be more robust and provide better error context.
constructor(private context: DefaultContextSchema) {
- if (!this.context.collectionFlow?.state) {
- throw new Error('Collection flow state is not set.');
+ if (!this.context.collectionFlow) {
+ throw new Error('Collection flow is not initialized in context');
+ }
+ if (!this.context.collectionFlow.state) {
+ throw new Error('Collection flow state is not initialized');
}
}📝 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.
| constructor(private context: DefaultContextSchema) { | |
| if (!this.context.collectionFlow?.state) { | |
| throw new Error('Collection flow state is not set.'); | |
| } | |
| } | |
| constructor(private context: DefaultContextSchema) { | |
| if (!this.context.collectionFlow) { | |
| throw new Error('Collection flow is not initialized in context'); | |
| } | |
| if (!this.context.collectionFlow.state) { | |
| throw new Error('Collection flow state is not initialized'); | |
| } | |
| } |
| override(state: NonNullable<NonNullable<DefaultContextSchema['collectionFlow']>['state']>) { | ||
| this.context.collectionFlow!.state = state; | ||
|
|
||
| console.log('StateHelper, context override', this.context.collectionFlow!.state); | ||
|
|
||
| return this.context; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify type definition and add state validation
The type definition is overly complex and the method lacks proper validation of the new state.
-override(state: NonNullable<NonNullable<DefaultContextSchema['collectionFlow']>['state']>) {
+type CollectionFlowState = Required<DefaultContextSchema['collectionFlow']>['state'];
+override(state: CollectionFlowState) {
+ // Validate required properties
+ if (!state.uiState || !state.collectionFlowState || !state.progress) {
+ throw new Error('Invalid state: missing required properties');
+ }
+
this.context.collectionFlow!.state = state;
- console.log('StateHelper, context override', this.context.collectionFlow!.state);
+ this.logger.info('State override completed', { newState: state });
return this.context;
}Committable suggestion was skipped due to low confidence.
| const uiDefinition = await prismaClient.uiDefinition.findFirst({ | ||
| where: { | ||
| workflowDefinitionId, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Add error handling for database operations
The database query lacks error handling and proper type checking for the result.
Consider adding proper error handling:
- const uiDefinition = await prismaClient.uiDefinition.findFirst({
- where: {
- workflowDefinitionId,
- },
- });
+ const uiDefinition = await prismaClient.uiDefinition.findFirst({
+ where: {
+ workflowDefinitionId,
+ },
+ }).catch(error => {
+ throw new Error(`Failed to fetch UI definition: ${error.message}`);
+ });
+
+ if (!uiDefinition) {
+ throw new Error(`UI definition not found for workflow ${workflowDefinitionId}`);
+ }📝 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 uiDefinition = await prismaClient.uiDefinition.findFirst({ | |
| where: { | |
| workflowDefinitionId, | |
| }, | |
| }); | |
| const uiDefinition = await prismaClient.uiDefinition.findFirst({ | |
| where: { | |
| workflowDefinitionId, | |
| }, | |
| }).catch(error => { | |
| throw new Error(`Failed to fetch UI definition: ${error.message}`); | |
| }); | |
| if (!uiDefinition) { | |
| throw new Error(`UI definition not found for workflow ${workflowDefinitionId}`); | |
| } |
| const collectionFlowManager = new CollectionFlowManager(initialContext, { | ||
| apiUrl: env.APP_API_URL, | ||
| steps: await getStepsInOrder(uiDefinition as UiDefinition), | ||
| }); | ||
|
|
||
| collectionFlowManager.initializeCollectionFlowContext(); | ||
|
|
There was a problem hiding this comment.
Add error handling for CollectionFlowManager operations
The initialization of CollectionFlowManager and its context lacks error handling.
Consider adding proper error handling:
- const collectionFlowManager = new CollectionFlowManager(initialContext, {
- apiUrl: env.APP_API_URL,
- steps: await getStepsInOrder(uiDefinition as UiDefinition),
- });
-
- collectionFlowManager.initializeCollectionFlowContext();
+ try {
+ const steps = await getStepsInOrder(uiDefinition);
+ if (!steps.length) {
+ throw new Error('No steps found in UI definition');
+ }
+
+ const collectionFlowManager = new CollectionFlowManager(initialContext, {
+ apiUrl: env.APP_API_URL,
+ steps,
+ });
+
+ collectionFlowManager.initializeCollectionFlowContext();
+ } catch (error) {
+ throw new Error(`Failed to initialize collection flow: ${error.message}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const collectionFlowManager = new CollectionFlowManager(initialContext, { | |
| apiUrl: env.APP_API_URL, | |
| steps: await getStepsInOrder(uiDefinition as UiDefinition), | |
| }); | |
| collectionFlowManager.initializeCollectionFlowContext(); | |
| try { | |
| const steps = await getStepsInOrder(uiDefinition); | |
| if (!steps.length) { | |
| throw new Error('No steps found in UI definition'); | |
| } | |
| const collectionFlowManager = new CollectionFlowManager(initialContext, { | |
| apiUrl: env.APP_API_URL, | |
| steps, | |
| }); | |
| collectionFlowManager.initializeCollectionFlowContext(); | |
| } catch (error) { | |
| throw new Error(`Failed to initialize collection flow: ${error.message}`); | |
| } |
| collectionFlow: z | ||
| .object({ | ||
| stepsProgress: z | ||
| .record( | ||
| z.string(), | ||
| z.object({ | ||
| // TODO Until backwards compatibility is handled | ||
| number: z.number().default(0), | ||
| isCompleted: z.boolean(), | ||
| }), | ||
| ) | ||
| .or(z.undefined()), | ||
| config: z.object({ | ||
| apiUrl: z.string().url(), | ||
| steps: z.array(z.object({ stateName: z.string(), orderNumber: z.number() })), | ||
| }), | ||
| state: z.object({ | ||
| uiState: z.string(), | ||
| collectionFlowState: z.enum(Object.values(CollectionFlowStates) as [string, ...string[]]), | ||
| progress: z.record(z.string(), z.object({ isCompleted: z.boolean() })), | ||
| }), | ||
| additionalInformation: z.record(z.string(), z.unknown()).optional(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance schema validation for better type safety and data integrity.
While the schema structure is good, consider adding these validations:
- Ensure steps array is non-empty
- Validate uniqueness of step order numbers
- Ensure state names are unique
- Validate that progress keys correspond to defined state names
Apply this diff to strengthen the schema:
collectionFlow: z
.object({
config: z.object({
apiUrl: z.string().url(),
- steps: z.array(z.object({ stateName: z.string(), orderNumber: z.number() })),
+ steps: z
+ .array(z.object({
+ stateName: z.string(),
+ orderNumber: z.number().int().nonnegative()
+ }))
+ .min(1, 'At least one step is required')
+ .refine(
+ steps => new Set(steps.map(s => s.orderNumber)).size === steps.length,
+ 'Step order numbers must be unique'
+ )
+ .refine(
+ steps => new Set(steps.map(s => s.stateName)).size === steps.length,
+ 'Step state names must be unique'
+ ),
}),
state: z.object({
uiState: z.string(),
collectionFlowState: z.enum(Object.values(CollectionFlowStates) as [string, ...string[]]),
- progress: z.record(z.string(), z.object({ isCompleted: z.boolean() })),
+ progress: z.record(
+ z.string(),
+ z.object({ isCompleted: z.boolean() })
+ ).refine(
+ (progress, ctx) => {
+ const stateNames = ctx.parent.config?.steps?.map(s => s.stateName) || [];
+ return Object.keys(progress).every(key => stateNames.includes(key));
+ },
+ 'Progress keys must correspond to defined state names'
+ ),
}),
additionalInformation: z.record(z.string(), z.unknown()).optional(),
})📝 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.
| collectionFlow: z | |
| .object({ | |
| stepsProgress: z | |
| .record( | |
| z.string(), | |
| z.object({ | |
| // TODO Until backwards compatibility is handled | |
| number: z.number().default(0), | |
| isCompleted: z.boolean(), | |
| }), | |
| ) | |
| .or(z.undefined()), | |
| config: z.object({ | |
| apiUrl: z.string().url(), | |
| steps: z.array(z.object({ stateName: z.string(), orderNumber: z.number() })), | |
| }), | |
| state: z.object({ | |
| uiState: z.string(), | |
| collectionFlowState: z.enum(Object.values(CollectionFlowStates) as [string, ...string[]]), | |
| progress: z.record(z.string(), z.object({ isCompleted: z.boolean() })), | |
| }), | |
| additionalInformation: z.record(z.string(), z.unknown()).optional(), | |
| collectionFlow: z | |
| .object({ | |
| config: z.object({ | |
| apiUrl: z.string().url(), | |
| steps: z | |
| .array(z.object({ | |
| stateName: z.string(), | |
| orderNumber: z.number().int().nonnegative() | |
| })) | |
| .min(1, 'At least one step is required') | |
| .refine( | |
| steps => new Set(steps.map(s => s.orderNumber)).size === steps.length, | |
| 'Step order numbers must be unique' | |
| ) | |
| .refine( | |
| steps => new Set(steps.map(s => s.stateName)).size === steps.length, | |
| 'Step state names must be unique' | |
| ), | |
| }), | |
| state: z.object({ | |
| uiState: z.string(), | |
| collectionFlowState: z.enum(Object.values(CollectionFlowStates) as [string, ...string[]]), | |
| progress: z.record( | |
| z.string(), | |
| z.object({ isCompleted: z.boolean() }) | |
| ).refine( | |
| (progress, ctx) => { | |
| const stateNames = ctx.parent.config?.steps?.map(s => s.stateName) || []; | |
| return Object.keys(progress).every(key => stateNames.includes(key)); | |
| }, | |
| 'Progress keys must correspond to defined state names' | |
| ), | |
| }), | |
| additionalInformation: z.record(z.string(), z.unknown()).optional(), | |
| }) |
| // Initializing Collection Flow | ||
| const collectionFlowManager = new CollectionFlowManager( | ||
| { | ||
| ...workflowRuntimeData.context, | ||
| }, | ||
| { | ||
| apiUrl: env.APP_API_URL, | ||
| steps: await getStepsInOrder(uiDefinition as UiDefinition), | ||
| additionalInformation: { | ||
| customerCompany: customer.displayName, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| collectionFlowManager.initializeCollectionFlowContext(); | ||
|
|
||
| workflowRuntimeData = await this.workflowRuntimeDataRepository.updateStateById( | ||
| workflowRuntimeData.id, | ||
| { | ||
| data: { | ||
| context: { | ||
| ...workflowRuntimeData.context, | ||
| ...collectionFlowManager.context, | ||
| metadata: { | ||
| ...(workflowRuntimeData.context.metadata ?? {}), | ||
| token: workflowToken.token, | ||
| collectionFlowUrl: env.COLLECTION_FLOW_URL, | ||
| webUiSDKUrl: env.WEB_UI_SDK_URL, | ||
| }, | ||
| }, | ||
| } as InputJsonValue, |
There was a problem hiding this comment.
Add error handling and consolidate context updates.
The collection flow initialization lacks error handling and has scattered context updates that could lead to inconsistent states.
Consider refactoring to:
- Add try-catch for CollectionFlowManager initialization
- Validate required configuration values
- Consolidate context updates
- // Initializing Collection Flow
- const collectionFlowManager = new CollectionFlowManager(
- {
- ...workflowRuntimeData.context,
- },
- {
- apiUrl: env.APP_API_URL,
- steps: await getStepsInOrder(uiDefinition as UiDefinition),
- additionalInformation: {
- customerCompany: customer.displayName,
- },
- },
- );
-
- collectionFlowManager.initializeCollectionFlowContext();
-
- workflowRuntimeData = await this.workflowRuntimeDataRepository.updateStateById(
- workflowRuntimeData.id,
- {
- data: {
- context: {
- ...collectionFlowManager.context,
- metadata: {
- ...(workflowRuntimeData.context.metadata ?? {}),
- token: workflowToken.token,
- collectionFlowUrl: env.COLLECTION_FLOW_URL,
- webUiSDKUrl: env.WEB_UI_SDK_URL,
- },
- } as InputJsonValue,
- projectId: currentProjectId,
- },
- },
- transaction,
- );
+ try {
+ if (!env.APP_API_URL || !env.COLLECTION_FLOW_URL || !env.WEB_UI_SDK_URL) {
+ throw new Error('Missing required environment variables for collection flow');
+ }
+
+ const collectionFlowManager = new CollectionFlowManager(
+ {
+ ...workflowRuntimeData.context,
+ },
+ {
+ apiUrl: env.APP_API_URL,
+ steps: await getStepsInOrder(uiDefinition as UiDefinition),
+ additionalInformation: {
+ customerCompany: customer.displayName,
+ },
+ },
+ );
+
+ collectionFlowManager.initializeCollectionFlowContext();
+
+ const updatedContext = {
+ ...collectionFlowManager.context,
+ metadata: {
+ ...(workflowRuntimeData.context.metadata ?? {}),
+ token: workflowToken.token,
+ collectionFlowUrl: env.COLLECTION_FLOW_URL,
+ webUiSDKUrl: env.WEB_UI_SDK_URL,
+ },
+ };
+
+ workflowRuntimeData = await this.workflowRuntimeDataRepository.updateStateById(
+ workflowRuntimeData.id,
+ {
+ data: {
+ context: updatedContext as InputJsonValue,
+ projectId: currentProjectId,
+ },
+ },
+ transaction,
+ );
+ } catch (error) {
+ this.logger.error('Failed to initialize collection flow', {
+ error,
+ workflowId: workflowRuntimeData.id,
+ });
+ throw new InternalServerErrorException('Failed to initialize collection flow');
+ }📝 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.
| // Initializing Collection Flow | |
| const collectionFlowManager = new CollectionFlowManager( | |
| { | |
| ...workflowRuntimeData.context, | |
| }, | |
| { | |
| apiUrl: env.APP_API_URL, | |
| steps: await getStepsInOrder(uiDefinition as UiDefinition), | |
| additionalInformation: { | |
| customerCompany: customer.displayName, | |
| }, | |
| }, | |
| ); | |
| collectionFlowManager.initializeCollectionFlowContext(); | |
| workflowRuntimeData = await this.workflowRuntimeDataRepository.updateStateById( | |
| workflowRuntimeData.id, | |
| { | |
| data: { | |
| context: { | |
| ...workflowRuntimeData.context, | |
| ...collectionFlowManager.context, | |
| metadata: { | |
| ...(workflowRuntimeData.context.metadata ?? {}), | |
| token: workflowToken.token, | |
| collectionFlowUrl: env.COLLECTION_FLOW_URL, | |
| webUiSDKUrl: env.WEB_UI_SDK_URL, | |
| }, | |
| }, | |
| } as InputJsonValue, | |
| try { | |
| if (!env.APP_API_URL || !env.COLLECTION_FLOW_URL || !env.WEB_UI_SDK_URL) { | |
| throw new Error('Missing required environment variables for collection flow'); | |
| } | |
| const collectionFlowManager = new CollectionFlowManager( | |
| { | |
| ...workflowRuntimeData.context, | |
| }, | |
| { | |
| apiUrl: env.APP_API_URL, | |
| steps: await getStepsInOrder(uiDefinition as UiDefinition), | |
| additionalInformation: { | |
| customerCompany: customer.displayName, | |
| }, | |
| }, | |
| ); | |
| collectionFlowManager.initializeCollectionFlowContext(); | |
| const updatedContext = { | |
| ...collectionFlowManager.context, | |
| metadata: { | |
| ...(workflowRuntimeData.context.metadata ?? {}), | |
| token: workflowToken.token, | |
| collectionFlowUrl: env.COLLECTION_FLOW_URL, | |
| webUiSDKUrl: env.WEB_UI_SDK_URL, | |
| }, | |
| }; | |
| workflowRuntimeData = await this.workflowRuntimeDataRepository.updateStateById( | |
| workflowRuntimeData.id, | |
| { | |
| data: { | |
| context: updatedContext as InputJsonValue, | |
| projectId: currentProjectId, | |
| }, | |
| }, | |
| transaction, | |
| ); | |
| } catch (error) { | |
| this.logger.error('Failed to initialize collection flow', { | |
| error, | |
| workflowId: workflowRuntimeData.id, | |
| }); | |
| throw new InternalServerErrorException('Failed to initialize collection flow'); | |
| } |
Summary by CodeRabbit
New Features
Improvements
Refactor
Tests