-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Prevent querying too much on worklow index page #14916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Summary
This PR addresses a performance optimization by preventing excessive querying on the workflow index page. The main change involves refactoring and renaming a GraphQL fields hook from `useRecordTableRecordGqlFields` to `useRecordFieldGqlFields` to make it more generic and reusable across different record view contexts (table, calendar, etc.).The key optimization is the addition of a special case for the 'workflowVersion' field that limits GraphQL queries to only fetch essential fields (id
and name
) instead of the full related object data. This prevents performance issues that were likely causing slow page loads on workflow-related pages.
The refactoring consolidates field selection logic into a single, well-optimized hook that can be used consistently across table views, calendar views, and other record display contexts. The changes also improve TypeScript type safety by adding proper type annotations and refactoring the field processing logic for better maintainability.
Changed Files
Filename | Score | Overview |
---|---|---|
packages/twenty-front/src/modules/object-record/record-index/hooks/useRecordIndexTableQuery.ts | 5/5 | Updated import path from useRecordTableRecordGqlFields to useRecordFieldGqlFields |
packages/twenty-front/src/modules/object-record/record-index/hooks/useRecordIndexTableFetchMore.ts | 5/5 | Updated import path from useRecordTableRecordGqlFields to useRecordFieldGqlFields |
packages/twenty-front/src/modules/object-record/record-calendar/components/RecordIndexCalendarDataLoaderEffect.tsx | 5/5 | Updated import path to use the renamed useRecordFieldGqlFields hook |
packages/twenty-front/src/modules/object-record/record-field/hooks/useRecordFieldGqlFields.ts | 4/5 | Renamed file with workflowVersion optimization and improved TypeScript typing |
Confidence score: 4/5
- This PR is safe to merge with minimal risk as it primarily involves renaming and import path updates
- Score reflects the straightforward nature of the refactoring with one targeted performance optimization
- The workflowVersion optimization logic needs attention to ensure it doesn't break existing functionality
Sequence Diagram
sequenceDiagram
participant User
participant "Record Index" as RI
participant "Record Calendar" as RC
participant "Record Field Hook" as RFH
participant "GraphQL" as GQL
participant "Record Store" as RS
User->>RI: "Navigate to record index page"
RI->>RFH: "useRecordFieldGqlFields(objectMetadataItem)"
RFH->>RFH: "Generate GQL fields from visible record fields"
Note over RFH: Special handling for workflowVersion field
RFH->>RFH: "Check if field is 'workflowVersion'"
RFH->>RFH: "Return lightweight {id: true, name: true} instead of full object"
RFH-->>RI: "Return optimized recordGqlFields"
RI->>GQL: "useFindManyRecords(params, recordGqlFields)"
GQL-->>RI: "Return records data"
RI->>RS: "upsertRecordsInStore(records)"
User->>RC: "Navigate to calendar view"
RC->>RFH: "useRecordFieldGqlFields(objectMetadataItem)"
RFH->>RFH: "Apply workflowVersion optimization"
RFH-->>RC: "Return optimized recordGqlFields"
RC->>GQL: "useFindManyRecords(params, limit: 100, recordGqlFields)"
GQL-->>RC: "Return records data"
RC->>RS: "upsertRecordsInStore(records)"
4 files reviewed, 1 comment
if (fieldMetadataItem.name === 'workflowVersion') { | ||
return [[fieldMetadataItem.name, { id: true, name: true }]]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: The workflowVersion check should be moved before the isDefined check to avoid potential null reference errors if fieldMetadataItem is undefined
if (fieldMetadataItem.name === 'workflowVersion') { | |
return [[fieldMetadataItem.name, { id: true, name: true }]]; | |
} | |
// TODO: remove this once we have made the workflowVersion lighter | |
if (fieldMetadataItem?.name === 'workflowVersion') { | |
return [[fieldMetadataItem.name, { id: true, name: true }]]; | |
} | |
if (!isDefined(fieldMetadataItem)) { | |
throw new Error( | |
`Field ${recordField.fieldMetadataItemId} is missing, please refresh the page. If the problem persists, please contact support.`, | |
); | |
} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/object-record/record-field/hooks/useRecordFieldGqlFields.ts
Line: 50:52
Comment:
**logic:** The workflowVersion check should be moved before the isDefined check to avoid potential null reference errors if fieldMetadataItem is undefined
```suggestion
// TODO: remove this once we have made the workflowVersion lighter
if (fieldMetadataItem?.name === 'workflowVersion') {
return [[fieldMetadataItem.name, { id: true, name: true }]];
}
if (!isDefined(fieldMetadataItem)) {
throw new Error(
`Field ${recordField.fieldMetadataItemId} is missing, please refresh the page. If the problem persists, please contact support.`,
);
}
```
How can I resolve this? If you propose a fix, please make it concise.
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:41196 This environment will automatically shut down when the PR is closed or after 5 hours. |
); | ||
} | ||
// TODO: remove this once we have made the workflowVersion lighter | ||
if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug Should be placed below invariant check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding optimistic cache might be returning partial workflow when we expect complete one ?
Left a comment !
Performance short term fix
Done in this PR:
workflowVersions and workflowRuns are heavy object. I'm preventing loading their data when they are loaded as relations. This will reduce the work on backend
Long term fix
Todo:
We should make sure workflow data is ligher