Skip to content

Commit af621cf

Browse files
icecrasher321icecrasher321
committed
address comments
1 parent fe1d901 commit af621cf

6 files changed

Lines changed: 95 additions & 18 deletions

File tree

‎apps/sim/app/api/chat/manage/[id]/route.test.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import {
1616
workflowsOrchestrationMock,
1717
workflowsOrchestrationMockFns,
1818
workflowsPersistenceUtilsMock,
19-
workflowsPersistenceUtilsMockFns,
2019
} from '@sim/testing'
2120
import { NextRequest } from 'next/server'
2221
import { beforeEach, describe, expect, it, vi } from 'vitest'
@@ -28,7 +27,7 @@ const { mockCheckChatAccess } = vi.hoisted(() => ({
2827
const mockCreateSuccessResponse = workflowsApiUtilsMockFns.mockCreateSuccessResponse
2928
const mockCreateErrorResponse = workflowsApiUtilsMockFns.mockCreateErrorResponse
3029
const mockEncryptSecret = encryptionMockFns.mockEncryptSecret
31-
const mockDeployWorkflow = workflowsPersistenceUtilsMockFns.mockDeployWorkflow
30+
const mockPerformFullDeploy = workflowsOrchestrationMockFns.mockPerformFullDeploy
3231
const mockPerformChatUndeploy = workflowsOrchestrationMockFns.mockPerformChatUndeploy
3332
const mockNotifySocketDeploymentChanged =
3433
workflowsOrchestrationMockFns.mockNotifySocketDeploymentChanged
@@ -73,7 +72,7 @@ describe('Chat Edit API Route', () => {
7372
})
7473

7574
mockEncryptSecret.mockResolvedValue({ encrypted: 'encrypted-password' })
76-
mockDeployWorkflow.mockResolvedValue({ success: true, version: 1 })
75+
mockPerformFullDeploy.mockResolvedValue({ success: true, version: 1 })
7776
mockNotifySocketDeploymentChanged.mockResolvedValue(undefined)
7877
})
7978

‎apps/sim/app/api/workflows/[id]/route.test.ts‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,12 @@ const mockGetWorkflowById = workflowsUtilsMockFns.mockGetWorkflowById
2727
const mockAuthorizeWorkflowByWorkspacePermission =
2828
workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission
2929
const mockPerformDeleteWorkflow = workflowsOrchestrationMockFns.mockPerformDeleteWorkflow
30-
const mockDbUpdate = vi.fn()
31-
const mockDbSelect = vi.fn()
30+
31+
const { mockDbUpdate, mockDbSelect, mockDbTransaction } = vi.hoisted(() => ({
32+
mockDbUpdate: vi.fn(),
33+
mockDbSelect: vi.fn(),
34+
mockDbTransaction: vi.fn(),
35+
}))
3236

3337
/**
3438
* Helper to set mock auth state consistently across getSession and hybrid auth.
@@ -65,6 +69,7 @@ vi.mock('@sim/db', () => ({
6569
db: {
6670
update: () => mockDbUpdate(),
6771
select: () => mockDbSelect(),
72+
transaction: mockDbTransaction,
6873
},
6974
workflow: {},
7075
}))
@@ -80,6 +85,18 @@ describe('Workflow By ID API Route', () => {
8085
})
8186

8287
mockLoadWorkflowFromNormalizedTables.mockResolvedValue(null)
88+
mockDbTransaction.mockImplementation(async (callback) =>
89+
callback({
90+
execute: vi.fn().mockResolvedValue(undefined),
91+
select: vi.fn().mockReturnValue({
92+
from: vi.fn().mockReturnValue({
93+
where: vi.fn().mockReturnValue({
94+
limit: vi.fn().mockResolvedValue([]),
95+
}),
96+
}),
97+
}),
98+
})
99+
)
83100
})
84101

85102
describe('GET /api/workflows/[id]', () => {

‎apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/deploy-modal.tsx‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -353,7 +353,7 @@ export function DeployModal({
353353
if (!isWorkflowStillActive(workflowId) || deployActionIdRef.current !== actionId) return
354354
setDeployWarnings([...(result.warnings || []), ...(syncWarning ? [syncWarning] : [])])
355355
} finally {
356-
if (deployActionIdRef.current === actionId && isWorkflowStillActive(workflowId)) {
356+
if (deployActionIdRef.current === actionId) {
357357
setIsFinalizingDeploy(false)
358358
}
359359
}
@@ -365,7 +365,7 @@ export function DeployModal({
365365
setDeployError(errorMessage)
366366
} finally {
367367
releaseDeployAction(workflowId)
368-
if (deployActionIdRef.current === actionId && isWorkflowStillActive(workflowId)) {
368+
if (deployActionIdRef.current === actionId) {
369369
setIsFinalizingDeploy(false)
370370
}
371371
}
@@ -461,7 +461,7 @@ export function DeployModal({
461461
if (!isWorkflowStillActive(workflowId) || deployActionIdRef.current !== actionId) return
462462
setDeployWarnings([...(result.warnings || []), ...(syncWarning ? [syncWarning] : [])])
463463
} finally {
464-
if (deployActionIdRef.current === actionId && isWorkflowStillActive(workflowId)) {
464+
if (deployActionIdRef.current === actionId) {
465465
setIsFinalizingDeploy(false)
466466
}
467467
}
@@ -473,7 +473,7 @@ export function DeployModal({
473473
setDeployError(errorMessage)
474474
} finally {
475475
releaseDeployAction(workflowId)
476-
if (deployActionIdRef.current === actionId && isWorkflowStillActive(workflowId)) {
476+
if (deployActionIdRef.current === actionId) {
477477
setIsFinalizingDeploy(false)
478478
}
479479
}

‎apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-readiness.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,10 +134,11 @@ export function useDeployReadiness(workflowId: string | null): DeployReadiness {
134134
const drained = await queue.waitForWorkflowOperations(workflowId)
135135
if (!drained) return false
136136

137+
const latestQueue = useOperationQueueStore.getState()
137138
const diff = useWorkflowDiffStore.getState()
138139
return (
139-
!queue.hasOperationError &&
140-
!queue.hasPendingOperations(workflowId) &&
140+
!latestQueue.hasOperationError &&
141+
!latestQueue.hasPendingOperations(workflowId) &&
141142
!diff.hasActiveDiff &&
142143
!diff.pendingExternalUpdates[workflowId] &&
143144
!diff.reconcilingWorkflows[workflowId] &&

‎apps/sim/lib/workflows/persistence/utils.test.ts‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -798,6 +798,58 @@ describe('Database Helpers', () => {
798798
})
799799
})
800800

801+
describe('workflow row locking', () => {
802+
function createMissingWorkflowTx() {
803+
const lockFor = vi.fn().mockResolvedValue([])
804+
const limit = vi.fn(() => ({ for: lockFor }))
805+
const where = vi.fn(() => ({ limit }))
806+
const from = vi.fn(() => ({ where }))
807+
const select = vi.fn(() => ({ from }))
808+
const update = vi.fn()
809+
810+
return {
811+
tx: {
812+
execute: vi.fn().mockResolvedValue([{ id: mockWorkflowId }]),
813+
select,
814+
update,
815+
},
816+
lockFor,
817+
update,
818+
}
819+
}
820+
821+
it('returns not_found when deploy cannot lock a workflow row', async () => {
822+
const { tx, lockFor } = createMissingWorkflowTx()
823+
mockDb.transaction = vi.fn().mockImplementation(async (callback) => callback(tx))
824+
825+
const result = await dbHelpers.deployWorkflow({
826+
workflowId: mockWorkflowId,
827+
deployedBy: 'user-123',
828+
})
829+
830+
expect(result).toEqual({
831+
success: false,
832+
error: 'Workflow not found',
833+
errorCode: 'not_found',
834+
})
835+
expect(lockFor).toHaveBeenCalledWith('update')
836+
expect(tx.execute).not.toHaveBeenCalled()
837+
})
838+
839+
it('returns an error when undeploy cannot lock a workflow row', async () => {
840+
const { tx, update } = createMissingWorkflowTx()
841+
mockDb.transaction = vi.fn().mockImplementation(async (callback) => callback(tx))
842+
843+
const result = await dbHelpers.undeployWorkflow({ workflowId: mockWorkflowId })
844+
845+
expect(result).toEqual({
846+
success: false,
847+
error: 'Workflow not found',
848+
})
849+
expect(update).not.toHaveBeenCalled()
850+
})
851+
})
852+
801853
describe('error handling and edge cases', () => {
802854
it('should handle very large workflow data', async () => {
803855
const blocks: Record<string, ReturnType<typeof createBlock>> = {}

‎apps/sim/lib/workflows/persistence/utils.ts‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,18 @@ const logger = createLogger('WorkflowDBHelpers')
2525
export type { DbOrTx, NormalizedWorkflowData } from '@sim/workflow-persistence/types'
2626
export type WorkflowDeploymentVersion = InferSelectModel<typeof workflowDeploymentVersion>
2727

28-
async function lockWorkflowForUpdate(tx: DbOrTx, workflowId: string): Promise<boolean> {
29-
if ('execute' in tx && typeof tx.execute === 'function') {
30-
await tx.execute(sql`SELECT id FROM workflow WHERE id = ${workflowId} FOR UPDATE`)
31-
return true
28+
function hasReturnedRows(result: unknown): boolean {
29+
if (Array.isArray(result)) return result.length > 0
30+
31+
if (result && typeof result === 'object') {
32+
const rows = 'rows' in result ? result.rows : undefined
33+
if (Array.isArray(rows)) return rows.length > 0
3234
}
3335

36+
return Boolean(result)
37+
}
38+
39+
async function lockWorkflowForUpdate(tx: DbOrTx, workflowId: string): Promise<boolean> {
3440
const query = tx.select({ id: workflow.id }).from(workflow).where(eq(workflow.id, workflowId))
3541

3642
if ('limit' in query && typeof query.limit === 'function') {
@@ -39,12 +45,12 @@ async function lockWorkflowForUpdate(tx: DbOrTx, workflowId: string): Promise<bo
3945
'for' in limited && typeof limited.for === 'function'
4046
? await limited.for('update')
4147
: await limited
42-
return Array.isArray(rows) ? rows.length > 0 : true
48+
return hasReturnedRows(rows)
4349
}
4450

4551
const rows = await query
4652

47-
return Array.isArray(rows) ? rows.length > 0 : true
53+
return hasReturnedRows(rows)
4854
}
4955

5056
export interface WorkflowDeploymentVersionResponse {
@@ -816,7 +822,9 @@ export async function undeployWorkflow(params: {
816822
const { workflowId, tx } = params
817823

818824
const executeUndeploy = async (dbCtx: DbOrTx) => {
819-
await lockWorkflowForUpdate(dbCtx, workflowId)
825+
if (!(await lockWorkflowForUpdate(dbCtx, workflowId))) {
826+
throw new Error('Workflow not found')
827+
}
820828

821829
const deploymentVersions = await dbCtx
822830
.select({ id: workflowDeploymentVersion.id })

0 commit comments

Comments
 (0)