Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
5 issues found across 24 files
Prompt for AI agents (all 5 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-front/src/modules/workflow/workflow-variables/utils/generate/generateRecordEventOutputSchema.ts">
<violation number="1" location="packages/twenty-front/src/modules/workflow/workflow-variables/utils/generate/generateRecordEventOutputSchema.ts:178">
P2: Switch statement does not handle all `DatabaseEventAction` values (`RESTORED` and `UPSERTED` are missing). These cases silently fall through to the default, which may mask unintended behavior. Consider explicitly handling all cases or using a TypeScript exhaustiveness check.</violation>
</file>
<file name="packages/twenty-front/src/modules/workflow/workflow-variables/utils/generate/generateFakeValue.ts">
<violation number="1" location="packages/twenty-front/src/modules/workflow/workflow-variables/utils/generate/generateFakeValue.ts:38">
P2: Using `split(':')` to parse property types is fragile. If the value type contains a colon (e.g., for nested object types), only the first two parts will be captured, corrupting the type definition. Consider using `split(':')` with a limit or a different parsing strategy.</violation>
</file>
<file name="packages/twenty-front/src/modules/workflow/workflow-variables/utils/generate/generateRecordOutputSchema.ts">
<violation number="1" location="packages/twenty-front/src/modules/workflow/workflow-variables/utils/generate/generateRecordOutputSchema.ts:28">
P2: [gpt-5.2] `isActive` is nullable/optional on `FieldMetadataItem`, but this falsy check treats `null`/`undefined` as inactive and will exclude fields unexpectedly. Prefer an explicit `=== false` check so only explicitly disabled fields are excluded.</violation>
<violation number="2" location="packages/twenty-front/src/modules/workflow/workflow-variables/utils/generate/generateRecordOutputSchema.ts:117">
P2: [gpt-5.2] Unconditionally writing `${relationName}Id` can overwrite an existing field with the same name (order-dependent), producing an unstable/incorrect schema. Guard against collisions and only add the synthetic id field when it doesn’t already exist.</violation>
</file>
<file name="packages/twenty-front/src/modules/workflow/workflow-variables/hooks/useStepsOutputSchema.ts">
<violation number="1" location="packages/twenty-front/src/modules/workflow/workflow-variables/hooks/useStepsOutputSchema.ts:47">
P1: Inconsistent handling of backend-computed schemas for steps vs triggers. For steps with types like CODE, HTTP_REQUEST, AI_AGENT, WEBHOOK, ITERATOR, `computeStepOutputSchema` returns `undefined`, resulting in an empty output schema. The trigger handling correctly falls back to `trigger.settings?.outputSchema`. Steps should use the same pattern: `shouldComputeOnFrontend ? computeStepOutputSchema({...}) : step.settings?.outputSchema`.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
...nt/src/modules/workflow/workflow-variables/utils/generate/generateRecordEventOutputSchema.ts
Show resolved
Hide resolved
...ges/twenty-front/src/modules/workflow/workflow-variables/utils/generate/generateFakeValue.ts
Show resolved
Hide resolved
...y-front/src/modules/workflow/workflow-variables/utils/generate/generateRecordOutputSchema.ts
Show resolved
Hide resolved
...y-front/src/modules/workflow/workflow-variables/utils/generate/generateRecordOutputSchema.ts
Show resolved
Hide resolved
packages/twenty-front/src/modules/workflow/workflow-variables/hooks/useStepsOutputSchema.ts
Outdated
Show resolved
Hide resolved
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:32026 This environment will automatically shut down when the PR is closed or after 5 hours. |
martmull
left a comment
There was a problem hiding this comment.
Hey nice changes. I see that some code has been duplicated I think it worth factorize directly using twenty-shared when possible
| }): OutputSchemaV2 | undefined => { | ||
| const stepType = step.type; | ||
|
|
||
| if (PERSISTED_OUTPUT_SCHEMA_TYPES.includes(stepType)) { |
There was a problem hiding this comment.
use utils shouldComputeOutput...
| @@ -25,6 +27,7 @@ export type FieldOutputSchemaV2 = RecordFieldLeaf | RecordFieldNode; | |||
|
|
|||
| export type RecordOutputSchemaV2 = { | |||
| object: { | |||
| icon?: string; | |||
| label: string; | |||
| objectMetadataId: string; | |||
| isRelationField?: boolean; | |||
There was a problem hiding this comment.
Would be great to factorize all those types in twenty-shared (in another PR obviously)
| import { FieldMetadataType } from 'twenty-shared/types'; | ||
|
|
||
| export type FakeValueTypes = | ||
| | string | ||
| | number | ||
| | boolean | ||
| | Date | ||
| | FakeValueTypes[] | ||
| | FieldMetadataType | ||
| | { [key: string]: FakeValueTypes } | ||
| | null; | ||
|
|
||
| type TypeClassification = 'Primitive' | 'FieldMetadataType'; | ||
|
|
||
| const generatePrimitiveValue = (valueType: string): FakeValueTypes => { | ||
| if (valueType === 'string') { | ||
| return 'My text'; | ||
| } else if (valueType === 'number') { | ||
| return 20; | ||
| } else if (valueType === 'boolean') { | ||
| return true; | ||
| } else if (valueType === 'Date') { | ||
| return new Date(); | ||
| } else if (valueType.endsWith('[]')) { | ||
| const elementType = valueType.replace('[]', ''); | ||
|
|
||
| return Array.from({ length: 3 }, () => generateFakeValue(elementType)); | ||
| } else if (valueType.startsWith('{') && valueType.endsWith('}')) { | ||
| const objData: Record<string, FakeValueTypes> = {}; | ||
|
|
||
| const properties = valueType | ||
| .slice(1, -1) | ||
| .split(';') | ||
| .map((property) => property.trim()) |
There was a problem hiding this comment.
We should move that to twenty-shared instead of copy paste
| import { type FindRecordsOutputSchema } from '@/workflow/workflow-variables/types/FindRecordsOutputSchema'; | ||
| import { generateRecordOutputSchema } from '@/workflow/workflow-variables/utils/generate/generateRecordOutputSchema'; | ||
|
|
||
| export const generateFindRecordsOutputSchema = ( |
| const camelToTitleCase = (camelCaseText: string): string => | ||
| capitalize( | ||
| camelCaseText | ||
| .replace(/([A-Z])/g, ' $1') | ||
| .replace(/^./, (str) => str.toUpperCase()), | ||
| ); |
| const stepOutputSchema: StepOutputSchemaV2 = { | ||
| id: step.id, | ||
| name: step.name, | ||
| type: step.type, | ||
| icon: getActionIcon(step.type), | ||
| outputSchema: (outputSchema ?? {}) as OutputSchemaV2, | ||
| }; | ||
|
|
||
| set(stepsOutputSchemaFamilyState(stepKey), stepOutputSchema); | ||
| set(shouldRecomputeOutputSchemaFamilyState(stepKey), false); | ||
| }); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
21e1e2a to
3ff213d
Compare
3ff213d to
de31b8b
Compare
12b1d60 to
0d6d579
Compare
0d6d579 to
c7817cd
Compare
Fixes twentyhq/core-team-issues#1382
Current issue : all step output schemas are computed and stored on backend side. Which means that, when the database schema is updated - like a field creation - steps needs to be deleted an recreated. Which is invisible to users.
Solution : schema generation is moved on frontend side
Coming on the page the first time, the schema is populated for all steps except a few ones that are handled differently (Code, Webhook, http node, Agent)
A separated state allow to determine if a step needs a recomputation.
The user only needs a refresh to see the whole schema re-computed
Follow-up: