Skip to content

Commit 78e2c2b

Browse files
fix(review): integer rule rounding, success-gated workflow labels, display module hygiene
Second-pass review fixes: round integer rule fields so fractional input never reaches SQL LIMIT, gate workflow-name readiness on a successful non-placeholder load in both editor and preview (errored loads mislabeled valid workflows as deleted), lazily read the variables store in preview rows, move the filter-field JSON preview into the shared display module and unexport its single-consumer helpers, and align >= boundary copy (failure rate, error count, cooldown window) with implementation. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 433bb93 commit 78e2c2b

9 files changed

Lines changed: 125 additions & 53 deletions

File tree

apps/docs/content/docs/en/triggers/sim.mdx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,11 @@ Pick one event per Sim trigger block:
2020
<li><strong>Workflow Deployed</strong>: a watched workflow was deployed (including redeploys and version rollbacks)</li>
2121
</ul>
2222

23-
**Alert conditions** — evaluated as runs complete, with a cooldown so they fire at most once per window:
23+
**Alert conditions** — evaluated as runs complete, with a cooldown so they fire at most once per cooldown window:
2424

2525
<ul className="list-disc space-y-1 pl-6">
2626
<li><strong>Consecutive Failures</strong>: the last N runs all failed</li>
27-
<li><strong>Failure Rate</strong>: failure rate exceeds a percentage over a time window (minimum 5 runs)</li>
27+
<li><strong>Failure Rate</strong>: failure rate meets or exceeds a percentage over a time window (minimum 5 runs)</li>
2828
<li><strong>Latency Threshold</strong>: a run took longer than a fixed duration</li>
2929
<li><strong>Latency Spike</strong>: a run was a configurable percentage slower than the recent average (minimum 5 runs)</li>
3030
<li><strong>Cost Threshold</strong>: a run cost more than a credit threshold</li>

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/workflow-block/workflow-block.tsx

Lines changed: 20 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,13 @@ import { calculateWorkflowBlockDimensions } from '@/lib/workflows/blocks/determi
1515
import { getConditionRows, getRouterRows } from '@/lib/workflows/dynamic-handle-topology'
1616
import {
1717
getDisplayValue,
18-
isPlainObject,
1918
resolveDropdownLabel,
19+
resolveFilterFieldLabel,
2020
resolveSkillsLabel,
2121
resolveToolsLabel,
2222
resolveVariablesLabel,
2323
resolveWorkflowMultiSelectLabel,
2424
resolveWorkflowSelectionLabel,
25-
tryParseJson,
2625
} from '@/lib/workflows/subblocks/display'
2726
import {
2827
buildCanonicalIndex,
@@ -246,16 +245,26 @@ const SubBlockRow = memo(function SubBlockRow({
246245
)
247246
const knowledgeBaseDisplayName = kbForDisplayName?.name ?? null
248247

249-
const { data: workflowMapForLookup = {}, isPending: workflowMapPending } =
250-
useWorkflowMap(workspaceId)
251-
/** Hydrates workflow-selector values and multi-select workflow dropdowns to names. */
248+
const {
249+
data: workflowMapForLookup = {},
250+
isSuccess: workflowMapLoaded,
251+
isPlaceholderData: workflowMapIsPlaceholder,
252+
} = useWorkflowMap(workspaceId)
253+
/**
254+
* Hydrates workflow-selector values and multi-select workflow dropdowns to
255+
* names. Ready only on a successful, non-placeholder load — an errored or
256+
* stale-placeholder map must not mislabel valid workflows as deleted.
257+
*/
252258
const workflowSelectionName = useMemo(() => {
253-
const lookup = { workflowMap: workflowMapForLookup, ready: !workflowMapPending }
259+
const lookup = {
260+
workflowMap: workflowMapForLookup,
261+
ready: workflowMapLoaded && !workflowMapIsPlaceholder,
262+
}
254263
return (
255264
resolveWorkflowSelectionLabel(subBlock, rawValue, lookup) ??
256265
resolveWorkflowMultiSelectLabel(subBlock, rawValue, lookup)
257266
)
258-
}, [workflowMapForLookup, workflowMapPending, subBlock, rawValue])
267+
}, [workflowMapForLookup, workflowMapLoaded, workflowMapIsPlaceholder, subBlock, rawValue])
259268

260269
const { data: mcpServers = [] } = useMcpServers(workspaceId || '')
261270
const mcpServerDisplayName = useMemo(() => {
@@ -329,26 +338,10 @@ const SubBlockRow = memo(function SubBlockRow({
329338
[subBlock, rawValue, customTools]
330339
)
331340

332-
const filterDisplayValue = useMemo(() => {
333-
const isFilterField =
334-
subBlock?.id === 'filter' || subBlock?.id === 'filterCriteria' || subBlock?.id === 'sort'
335-
336-
if (!isFilterField || !rawValue) return null
337-
338-
const parsedValue = tryParseJson(rawValue)
339-
340-
if (isPlainObject(parsedValue) || Array.isArray(parsedValue)) {
341-
try {
342-
const jsonStr = JSON.stringify(parsedValue, null, 0)
343-
if (jsonStr.length <= 35) return jsonStr
344-
return `${jsonStr.slice(0, 32)}...`
345-
} catch {
346-
return null
347-
}
348-
}
349-
350-
return null
351-
}, [subBlock?.id, rawValue])
341+
const filterDisplayValue = useMemo(
342+
() => resolveFilterFieldLabel(subBlock, rawValue),
343+
[subBlock, rawValue]
344+
)
352345

353346
/** Hydrates skill references to display names. */
354347
const { data: workspaceSkills = [] } = useSkills(workspaceId || '')

apps/sim/app/workspace/[workspaceId]/w/components/preview/components/preview-workflow/components/block/block.tsx

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -113,11 +113,15 @@ const SubBlockRow = memo(function SubBlockRow({
113113

114114
const workflowLookup = { workflowMap, ready: workflowLabelsReady }
115115
const dropdownLabel = resolveDropdownLabel(subBlock, rawValue)
116-
const variablesDisplay = resolveVariablesLabel(
117-
subBlock,
118-
rawValue,
119-
Object.values(useVariablesStore.getState().variables)
120-
)
116+
// Materialize the variables store only for variables-input rows.
117+
const variablesDisplay =
118+
subBlock?.type === 'variables-input'
119+
? resolveVariablesLabel(
120+
subBlock,
121+
rawValue,
122+
Object.values(useVariablesStore.getState().variables)
123+
)
124+
: null
121125
// The preview is hook-free, so custom tools referenced only by id resolve
122126
// through their inline schema/registry fallbacks rather than the API.
123127
const toolsDisplay = resolveToolsLabel(subBlock, rawValue, [])

apps/sim/app/workspace/[workspaceId]/w/components/preview/components/preview-workflow/preview-workflow.tsx

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -234,10 +234,12 @@ export function PreviewWorkflow({
234234
const workspaceId = propWorkspaceId ?? params.workspaceId
235235
const {
236236
data: workflowMap = {},
237-
isLoading: isWorkflowMapLoading,
237+
isSuccess: isWorkflowMapLoaded,
238238
isPlaceholderData: isWorkflowMapPlaceholderData,
239239
} = useWorkflowMap(workspaceId)
240-
const workflowLabelsReady = !isWorkflowMapLoading && !isWorkflowMapPlaceholderData
240+
// Ready only on a successful, non-placeholder load — an errored or stale
241+
// placeholder map must not mislabel valid workflows as deleted.
242+
const workflowLabelsReady = isWorkflowMapLoaded && !isWorkflowMapPlaceholderData
241243
const containerRef = useRef<HTMLDivElement>(null)
242244
const nodeTypes = previewNodeTypes
243245
const isValidWorkflowState = workflowState?.blocks && workflowState.edges

apps/sim/lib/workflows/subblocks/display.test.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ vi.mock('@/blocks', () => ({
99

1010
import {
1111
getDisplayValue,
12+
resolveDropdownLabel,
13+
resolveFilterFieldLabel,
1214
resolveSkillsLabel,
1315
resolveToolsLabel,
1416
resolveVariablesLabel,
@@ -130,6 +132,37 @@ describe('resolveSkillsLabel', () => {
130132
})
131133
})
132134

135+
describe('resolveDropdownLabel', () => {
136+
const dropdown = {
137+
id: 'mode',
138+
type: 'dropdown',
139+
options: [{ id: 'opt-1', label: 'Option One' }, 'literal'],
140+
} as SubBlockConfig
141+
142+
it('resolves option ids and string options to labels', () => {
143+
expect(resolveDropdownLabel(dropdown, 'opt-1')).toBe('Option One')
144+
expect(resolveDropdownLabel(dropdown, 'literal')).toBe('literal')
145+
expect(resolveDropdownLabel(dropdown, 'missing')).toBeNull()
146+
})
147+
})
148+
149+
describe('resolveFilterFieldLabel', () => {
150+
const filterField = { id: 'filter', type: 'short-input' } as SubBlockConfig
151+
152+
it('renders compact JSON for filter fields and truncates long values', () => {
153+
expect(resolveFilterFieldLabel(filterField, '{"a":1}')).toBe('{"a":1}')
154+
const long = JSON.stringify({ column: 'status', operator: 'contains', value: 'running' })
155+
expect(resolveFilterFieldLabel(filterField, long)).toBe(`${long.slice(0, 32)}...`)
156+
})
157+
158+
it('returns null for non-filter subblocks and non-JSON values', () => {
159+
expect(
160+
resolveFilterFieldLabel({ id: 'other', type: 'short-input' } as SubBlockConfig, '{"a":1}')
161+
).toBeNull()
162+
expect(resolveFilterFieldLabel(filterField, 'plain text')).toBeNull()
163+
})
164+
})
165+
133166
describe('getDisplayValue', () => {
134167
it('handles empty, scalar, and object values', () => {
135168
expect(getDisplayValue(null)).toBe('-')

apps/sim/lib/workflows/subblocks/display.ts

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,12 +76,12 @@ const isFieldFormatArray = (value: unknown): value is FieldFormat[] => {
7676
}
7777

7878
/** Checks if a value is a plain object (not array, not null). */
79-
export const isPlainObject = (value: unknown): value is Record<string, unknown> => {
79+
const isPlainObject = (value: unknown): value is Record<string, unknown> => {
8080
return typeof value === 'object' && value !== null && !Array.isArray(value)
8181
}
8282

8383
/** Type guard for variable assignments arrays (variables-input subblocks). */
84-
export const isVariableAssignmentsArray = (
84+
const isVariableAssignmentsArray = (
8585
value: unknown
8686
): value is Array<{ id?: string; variableId?: string; variableName?: string; value: unknown }> => {
8787
return (
@@ -167,7 +167,7 @@ const isSortConditionArray = (value: unknown): value is SortRule[] => {
167167
* Attempts to parse a JSON string, returning the parsed value or the
168168
* original value if parsing fails.
169169
*/
170-
export const tryParseJson = (value: unknown): unknown => {
170+
const tryParseJson = (value: unknown): unknown => {
171171
if (typeof value !== 'string') return value
172172
try {
173173
const trimmed = value.trim()
@@ -320,11 +320,35 @@ export const getDisplayValue = (value: unknown): string => {
320320
* `ready` gates resolution so missing entries only render as deleted once
321321
* the lookup has actually loaded.
322322
*/
323-
export interface WorkflowNameLookup {
323+
interface WorkflowNameLookup {
324324
workflowMap: Record<string, { name: string }>
325325
ready: boolean
326326
}
327327

328+
/**
329+
* Resolves filter/sort builder subblocks to a compact single-line JSON
330+
* preview. Returns null for other subblocks; callers use a non-null result
331+
* to apply monospace styling.
332+
*/
333+
export function resolveFilterFieldLabel(
334+
subBlock: SubBlockConfig | undefined,
335+
rawValue: unknown
336+
): string | null {
337+
const isFilterField =
338+
subBlock?.id === 'filter' || subBlock?.id === 'filterCriteria' || subBlock?.id === 'sort'
339+
if (!isFilterField || !rawValue) return null
340+
341+
const parsedValue = tryParseJson(rawValue)
342+
if (!isPlainObject(parsedValue) && !Array.isArray(parsedValue)) return null
343+
344+
try {
345+
const jsonStr = JSON.stringify(parsedValue, null, 0)
346+
return jsonStr.length <= 35 ? jsonStr : truncate(jsonStr, 32)
347+
} catch {
348+
return null
349+
}
350+
}
351+
328352
/**
329353
* Resolves a static dropdown/combobox value to its option label.
330354
* Returns null if not a dropdown/combobox or no matching option is found.

apps/sim/lib/workspace-events/subscriptions.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,4 +60,16 @@ describe('parseSubscriptionConfig', () => {
6060
expect(config?.errorCountThreshold).toBe(1000)
6161
expect(config?.inactivityHours).toBe(1)
6262
})
63+
64+
it('rounds fractional integer fields (counts feed SQL LIMIT) but keeps credits fractional', () => {
65+
const config = parseSubscriptionConfig({
66+
eventType: 'consecutive_failures',
67+
consecutiveFailures: '2.5',
68+
windowHours: 12.4,
69+
costThresholdCredits: 250.5,
70+
})
71+
expect(config?.consecutiveFailures).toBe(3)
72+
expect(config?.windowHours).toBe(12)
73+
expect(config?.costThresholdCredits).toBe(250.5)
74+
})
6375
})

apps/sim/lib/workspace-events/subscriptions.ts

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -66,28 +66,31 @@ function parseStringArray(value: unknown): string[] {
6666
* Per-field bounds ported from the legacy notifications contract. Rule SQL
6767
* runs on the execution-completion hot path, so windows and counts must stay
6868
* inside the designed envelope regardless of what the free-text subblocks
69-
* contain. The credit bounds are the legacy dollar bounds ($0.01-$1000) at
70-
* 200 credits per dollar.
69+
* contain. Integer fields are rounded — counts feed SQL LIMIT, which rejects
70+
* fractional values. The credit bounds are the legacy dollar bounds
71+
* ($0.01-$1000) at 200 credits per dollar; credits stay fractional like the
72+
* legacy dollar threshold.
7173
*/
7274
const SIM_RULE_BOUNDS = {
73-
consecutiveFailures: { min: 1, max: 100 },
74-
failureRatePercent: { min: 1, max: 100 },
75-
windowHours: { min: 1, max: 168 },
76-
durationThresholdMs: { min: 1000, max: 3_600_000 },
77-
latencySpikePercent: { min: 10, max: 1000 },
75+
consecutiveFailures: { min: 1, max: 100, integer: true },
76+
failureRatePercent: { min: 1, max: 100, integer: true },
77+
windowHours: { min: 1, max: 168, integer: true },
78+
durationThresholdMs: { min: 1000, max: 3_600_000, integer: true },
79+
latencySpikePercent: { min: 10, max: 1000, integer: true },
7880
costThresholdCredits: { min: 2, max: 200_000 },
79-
errorCountThreshold: { min: 1, max: 1000 },
80-
inactivityHours: { min: 1, max: 168 },
81+
errorCountThreshold: { min: 1, max: 1000, integer: true },
82+
inactivityHours: { min: 1, max: 168, integer: true },
8183
} as const
8284

8385
function parseBoundedNumber(
8486
value: unknown,
8587
fallback: number,
86-
bounds: { min: number; max: number }
88+
bounds: { min: number; max: number; integer?: boolean }
8789
): number {
8890
const parsed = typeof value === 'number' ? value : Number(value)
8991
if (!Number.isFinite(parsed) || parsed <= 0) return fallback
90-
return Math.min(Math.max(parsed, bounds.min), bounds.max)
92+
const clamped = Math.min(Math.max(parsed, bounds.min), bounds.max)
93+
return bounds.integer ? Math.round(clamped) : clamped
9194
}
9295

9396
/**

apps/sim/triggers/sim/workspace-event.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,8 @@ export const simWorkspaceEventTrigger: TriggerConfig = {
6969
type: 'short-input',
7070
placeholder: String(SIM_RULE_DEFAULTS.failureRatePercent),
7171
defaultValue: String(SIM_RULE_DEFAULTS.failureRatePercent),
72-
description: 'Fire when the failure rate exceeds this percentage over the time window.',
72+
description:
73+
'Fire when the failure rate meets or exceeds this percentage over the time window.',
7374
required: { field: 'eventType', value: 'failure_rate' },
7475
mode: 'trigger',
7576
condition: { field: 'eventType', value: 'failure_rate' },
@@ -114,7 +115,7 @@ export const simWorkspaceEventTrigger: TriggerConfig = {
114115
type: 'short-input',
115116
placeholder: String(SIM_RULE_DEFAULTS.errorCountThreshold),
116117
defaultValue: String(SIM_RULE_DEFAULTS.errorCountThreshold),
117-
description: 'Fire when this many errors occur within the time window.',
118+
description: 'Fire when at least this many errors occur within the time window.',
118119
required: { field: 'eventType', value: 'error_count' },
119120
mode: 'trigger',
120121
condition: { field: 'eventType', value: 'error_count' },

0 commit comments

Comments
 (0)