v0.3.29: copilot fixes, remove block from subflow, code cleanups#1004
Conversation
…#994) * improvement(db): remove deprecated column from workflow table * removed extraneous logs * update sockets envvar
…switching b/w logs (#1001)
* Fix abort * Cred updates * Updates * Fix sheet id showing up in diff view * Update diff view * Text overflow * Optimistic accept * Serialization catching * Depth 0 fix * Fix icons * Updates * Lint
…figs/state (#1000) * fix(workflow-error): allow users to delete workflows with invalid configs/state * cleanup
…o consolidate subflow code (#983) * added logic to remove blocks from subflows * refactored logic into just subflow-node * bun run lint * added subflow test * added a safety check for data.parentId * added state update logic * bun run lint * removed old logic * removed any * added tests * added type safety * removed test script * type safety --------- Co-authored-by: Adam Gough <adamgough@Mac.attlocal.net> Co-authored-by: waleedlatif1 <walif6@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Greptile Summary
This PR represents a significant architectural evolution for the workflow management system, consolidating multiple improvements across database schema, UI components, and the copilot feature. The most substantial change is the removal of the deprecated 'state' column from the workflow table, completing a migration from storing workflow data as JSON blobs to a normalized relational structure using separate tables (workflow_blocks, workflow_edges, workflow_subflows). This change affects numerous API endpoints, migration scripts, and database operations throughout the codebase.
The subflow consolidation (#983) is another major refactoring that merges previously separate LoopNodeComponent and ParallelNodeComponent into a unified SubflowNodeComponent. This new component uses a kind property to differentiate between 'loop' and 'parallel' types, reducing code duplication and improving maintainability. The consolidation includes updates to type definitions, CSS selectors, utility functions, and component mappings throughout the workflow editor.
The copilot diff improvements (#1002) introduce sophisticated workflow comparison capabilities. New type definitions in lib/workflows/diff/types.ts support tracking field-level changes, while components like WorkflowBlock and SubBlock now support diff mode with visual highlighting and interactive controls that don't modify the underlying workflow state. The diff system includes validation using serializer round-trip testing to prevent invalid configurations from reaching the canvas.
Additional improvements include logs cleanup (#999) with restructured workflow_execution_logs table that consolidates cost tracking into JSONB columns, OAuth credential refresh functionality for better copilot reliability, and various UI enhancements including improved text wrapping and terminology changes.
Confidence score: 3/5
- This PR introduces several breaking changes that could affect workflows not yet migrated to normalized tables, with multiple endpoints now returning 400 errors instead of fallback data
- Score reflects the significant architectural changes and removal of comprehensive test coverage for loop/parallel components, despite the improvements in code organization
- Pay close attention to migration files 0075 and 0076, API endpoints removing JSON blob fallbacks, and the new SubflowNodeComponent implementation
40 files reviewed, 10 comments
| field_diffs?: Record<string, { changed_fields: string[]; unchanged_fields: string[] }> | ||
| } | ||
|
|
||
| export function hasDiffStatus(block: any): block is BlockWithDiff { |
There was a problem hiding this comment.
style: Consider using a more specific parameter type instead of any for better type safety
| export function hasDiffStatus(block: any): block is BlockWithDiff { | |
| export function hasDiffStatus(block: unknown): block is BlockWithDiff { |
Context Used: Context - Avoid using type assertions to 'any' in TypeScript. Instead, ensure proper type definitions are used to maintain type safety. (link)
| return NextResponse.json({ data: finalWorkflowData }, { status: 200 }) | ||
| return NextResponse.json({ data: finalWorkflowData }, { status: 200 }) | ||
| } | ||
| return NextResponse.json({ error: 'Workflow has no normalized data' }, { status: 400 }) |
There was a problem hiding this comment.
logic: Breaking change: workflows without normalized data now return 400 error instead of falling back to JSON blob state. Ensure all workflows are migrated before deploying this change.
| acc as any, | ||
| acc.id |
There was a problem hiding this comment.
style: The type assertion acc as any bypasses TypeScript safety. Consider defining proper types for the credential parameter.
Context Used: Context - Avoid using type assertions to 'any' in TypeScript. Instead, ensure proper type definitions are used to maintain type safety. (link)
| } catch (_error) { | ||
| // If refresh fails, we still return whatever we had (may be null) | ||
| } |
There was a problem hiding this comment.
style: Empty catch block silently ignores refresh errors. Consider logging the error for debugging purposes.
| totalCost: (log.cost as any)?.total ?? null, | ||
| totalTokens: (log.cost as any)?.tokens?.total ?? null, |
There was a problem hiding this comment.
style: The type assertions to 'any' should be replaced with proper type definitions for the cost object structure
Context Used: Context - Avoid using type assertions to 'any' in TypeScript. Instead, ensure proper type definitions are used to maintain type safety. (link)
| it.concurrent('should be a memoized component', () => { | ||
| expect(SubflowNodeComponent).toBeDefined() | ||
| }) |
There was a problem hiding this comment.
style: Test doesn't verify memoization - it only checks if component exists
| it.concurrent('should handle delete button click logic (simulated)', () => { | ||
| const mockEvent = { stopPropagation: vi.fn() } | ||
|
|
||
| const handleDelete = (e: any, nodeId: string) => { | ||
| e.stopPropagation() | ||
| mockRemoveBlock(nodeId) | ||
| } | ||
|
|
||
| handleDelete(mockEvent, 'test-id') | ||
|
|
||
| expect(mockEvent.stopPropagation).toHaveBeenCalled() | ||
| expect(mockRemoveBlock).toHaveBeenCalledWith('test-id') | ||
| }) |
There was a problem hiding this comment.
style: Testing implementation details rather than actual component behavior - consider using render testing instead
| const fieldDiff = | ||
| currentWorkflow.isDiffMode && currentBlock ? (currentBlock as any).field_diffs : undefined | ||
| currentWorkflow.isDiffMode && currentBlock && hasDiffStatus(currentBlock) | ||
| ? currentBlock.field_diffs?.[id] |
There was a problem hiding this comment.
logic: Field diff access uses block ID as key, but the logic suggests field_diffs should be keyed by field name, not block ID. Verify the data structure matches this access pattern.
| const effectiveAdvanced = currentWorkflow.isDiffMode ? displayAdvancedMode : isAdvancedMode | ||
| const effectiveTrigger = currentWorkflow.isDiffMode ? displayTriggerMode : isTriggerMode |
There was a problem hiding this comment.
style: Consider extracting display mode calculations into a helper function to reduce code duplication and improve maintainability.
Context Used: Context - If a switch statement is large and handles multiple cases, extract each case into separate functions for better maintainability. (link)
| subBlockValues={ | ||
| data.subBlockValues || | ||
| (currentWorkflow.isDiffMode && currentBlock | ||
| ? (currentBlock as any).subBlocks |
There was a problem hiding this comment.
syntax: Type assertion (currentBlock as any).subBlocks bypasses TypeScript safety. Define proper types for diff-enhanced blocks.
Context Used: Context - Avoid using type assertions to 'any' in TypeScript. Instead, ensure proper type definitions are used to maintain type safety. (link)
v0.3.29: copilot fixes, remove block from subflow, code cleanups
Summary
Checklist