[Requires "warm" cache flush (no immediate downtime before flush)] Migrate viewGroup.fieldMetadataId -> view.mainGroupByFieldMetadataId (1/3)#16206
Conversation
ijreilly
changed the title
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| } | ||
|
|
||
| if (type === ViewType.Kanban) { | ||
| if (!isDefined(kanbanFieldMetadataId)) { | ||
| if (!isDefined(mainGroupByFieldMetadataId)) { | ||
| throw new Error('Kanban view must have a kanban field'); | ||
| } |
There was a problem hiding this comment.
Bug: Duplicating old Kanban views before backfill completes crashes due to mainGroupByFieldMetadataId being null.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The mainGroupByFieldMetadataId can be null for existing Kanban views after deployment but before the backfill command has completed. When a user attempts to duplicate such an old Kanban view using the 'create-from-current' mode, the sourceView.mainGroupByFieldMetadataId (which is null) is passed. This null value then triggers an error at lines 182-184, if (!isDefined(mainGroupByFieldMetadataId)) { throw new Error('Kanban view must have a kanban field'); }, leading to a crash in the view creation flow. This bug occurs during the critical transition period between deployment and the completion of the backfill operation.
💡 Suggested Fix
Ensure mainGroupByFieldMetadataId is always defined when duplicating Kanban views, possibly by adding a fallback to a default field or by preventing duplication until the backfill is confirmed complete for the sourceView.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-front/src/modules/views/hooks/useCreateViewFromCurrentView.ts#L182-L187
Potential issue: The `mainGroupByFieldMetadataId` can be `null` for existing Kanban
views after deployment but before the backfill command has completed. When a user
attempts to duplicate such an old Kanban view using the 'create-from-current' mode, the
`sourceView.mainGroupByFieldMetadataId` (which is `null`) is passed. This `null` value
then triggers an error at lines 182-184, `if (!isDefined(mainGroupByFieldMetadataId)) {
throw new Error('Kanban view must have a kanban field'); }`, leading to a crash in the
view creation flow. This bug occurs during the critical transition period between
deployment and the completion of the backfill operation.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 4468079
There was a problem hiding this comment.
No it's not a problem because this reflects viewPickerMainGroupByFieldMetadataIdComponentState which is not nullable. What we should do is actually prevent this from being equal to '', which we will be able to do after we have done the migration
Greptile OverviewGreptile SummaryIntroduces Key Changes:
Migration Strategy: Issues Found:
Confidence Score: 2/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant GraphQL
participant Backend
participant Database
Note over Database: Migration Phase
Backend->>Database: Run migration 1764300000000
Database-->>Backend: Add mainGroupByFieldMetadataId column
Backend->>Database: Run migration 1764300000001
Database-->>Backend: Add foreign key constraint
Backend->>Database: Run backfill command
Database-->>Backend: Populate mainGroupByFieldMetadataId from viewGroups
Backend->>Database: Delete inconsistent viewGroups
Note over User,Database: View Creation Flow
User->>Frontend: Create new Kanban view
Frontend->>Frontend: Select mainGroupByFieldMetadataId
Frontend->>GraphQL: createCoreView mutation
GraphQL->>Backend: CreateViewInput with mainGroupByFieldMetadataId
Backend->>Database: INSERT view with mainGroupByFieldMetadataId
Backend->>Database: CREATE viewGroups with same fieldMetadataId
Database-->>Backend: View created
Backend-->>GraphQL: Return view data
GraphQL-->>Frontend: View data (mainGroupByFieldMetadataId commented out)
Frontend->>Frontend: mainGroupByFieldMetadataId is undefined
Note over User,Database: Temporary Restriction
User->>Frontend: Try to change Group By field
Frontend->>Frontend: "Group by" menu item disabled
Frontend-->>User: Cannot change field (prevents inconsistencies)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
...er/src/database/commands/upgrade-version-command/1-13/1-13-upgrade-version-command.module.ts
Outdated
Show resolved
Hide resolved
| openRecordIn | ||
| kanbanAggregateOperation | ||
| kanbanAggregateOperationFieldMetadataId | ||
| # mainGroupByFieldMetadataId |
There was a problem hiding this comment.
logic: The field is commented out but still referenced in multiple places (convertCoreViewToView.ts:41, getObjectMetadataItemViews.ts:19, useCreateViewFromCurrentView.ts:136-137). The codebase expects this field to exist but will receive undefined from GraphQL queries, which could cause runtime issues.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/views/graphql/fragments/viewFragment.ts
Line: 27:27
Comment:
**logic:** The field is commented out but still referenced in multiple places (`convertCoreViewToView.ts:41`, `getObjectMetadataItemViews.ts:19`, `useCreateViewFromCurrentView.ts:136-137`). The codebase expects this field to exist but will receive `undefined` from GraphQL queries, which could cause runtime issues.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
It won't because it accept null values for mainGroupByFieldMetadataId. I want to avoid querying it to avoid downtime until we have cleared the cache and as this will have no effect for now
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:29866 This environment will automatically shut down when the PR is closed or after 5 hours. |
| <MenuItem | ||
| focused={selectedItemId === 'GroupBy'} | ||
| onClick={() => onContentChange('recordGroupFields')} | ||
| disabled |
There was a problem hiding this comment.
temporarily disabling this group by field switch on an existing view
|
|
||
| const recordGroupDefinitionsFromViewGroups = viewGroups | ||
| .map((viewGroup) => { | ||
| if (viewGroup.fieldMetadataId !== selectFieldMetadataItem.id) { |
There was a problem hiding this comment.
temporary, will be removed, but helps fix the display issues we currently have on views where some viewGroup are on a different fieldMetadataId
| @@ -0,0 +1,20 @@ | |||
| import { type MigrationInterface, type QueryRunner } from 'typeorm'; | |||
|
|
|||
| export class AddMainGroupByFieldMetadataIdForeignKey1764300000001 | |||
| description: | ||
| 'Backfill mainGroupByFieldMetadataId on views and clean up inconsistent viewGroups', | ||
| }) | ||
| export class BackfillViewMainGroupByFieldMetadataIdCommand extends MigrationCommandRunner { |
There was a problem hiding this comment.
extends ActiveOrSuspendedCommandRunner
...s/upgrade-version-command/1-13/1-13-backfill-view-main-group-by-field-metadata-id.command.ts
Show resolved
Hide resolved
...s/upgrade-version-command/1-13/1-13-backfill-view-main-group-by-field-metadata-id.command.ts
Show resolved
Hide resolved
| return; | ||
| } | ||
|
|
||
| await updateView({ |
There was a problem hiding this comment.
this is required to make sure that newly created grouped table views (not kanban) have mainGroupByFieldMetadataId (silently) filled
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
packages/twenty-front/src/modules/object-record/record-index/hooks/useHandleRecordGroupField.ts
Fixed
Show fixed
Hide fixed
| viewPickerVisibilityCallbackState, | ||
| ); | ||
|
|
||
| const shouldCopyFiltersAndSortsAndAggregate = |
There was a problem hiding this comment.
const isCreateFromCurrent =
| type, | ||
| kanbanFieldMetadataId, | ||
| mainGroupByFieldMetadataId: | ||
| type === ViewType.Kanban ? mainGroupByFieldMetadataId : null, |
ijreilly
changed the title
…ByFieldMetadataId (2/3) (#16277) Should be merged once #16206 has been released + command run to prod In this PR - Remove usage of viewGroup.fieldMetadataId, both in BE and FE states. - But we still need to properly populate it until we fully remove viewGroup.fieldMetadataId from db and ORM entity (upcoming 3rd PR out of 3). fieldMetadataId was removed from CoreViewGroup type and CreateViewGroupInput and is determined BE-side based on the associated view's mainGroupByFieldMetadataId. **I expect this means a downtime on viewGroup creation, until both FE and BE are deployed and cache is flushed.** This seems acceptable to me as it only regards viewGroup creation. - this information is replaced by view.mainGroupByFieldMetadataID - Handle view group creation, update and deletion in the BE as a side-effect of a view creation, update or deletion. Optimistic effects are still used - Add validation at view creation or update regarding mainGroupByFieldMetadata Left to do in 3rd PR - Remove viewGroup.fieldMetadataId from db and ORM entity - Restore feature allowing to update an existing grouped view's group by field (already OK on BE side but need to rebuild FE optimistic)
- 新增 mainGroupByFieldMetadataId 欄位到 View entity - 新增資料庫遷移 1764680275312-addMainGroupByFieldMetadataId - 補齊 usePersistViewGroup.ts 的 createViewGroups、deleteViewGroups、destroyViewGroups 函數 - 啟用 viewFragment.ts 的 mainGroupByFieldMetadataId 查詢 - 新增 isPersistingViewFieldsState 狀態 - 更新 ViewPicker 相關組件支援 mainGroupByFieldMetadataId
2 participants
In this PR (1/3)
In a next PR