Open
Conversation
Contributor
TODOs/FIXMEs:
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
packages/twenty-server/src/engine/metadata-modules/view-sort/resolvers/view-sort.resolver.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/metadata-modules/view-sort/resolvers/view-sort.resolver.ts
Outdated
Show resolved
Hide resolved
Contributor
Greptile OverviewGreptile SummaryThis PR refactors the view sort system to use the v2 migration architecture, bringing it in line with other metadata entities like view filters and view fields. Key Changes
Issues Found
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as Frontend Client
participant Resolver as ViewSortResolver
participant Service as ViewSortService
participant Migration as WorkspaceMigration<br/>ValidateBuildAndRun
participant Validator as FlatViewSortValidator
participant Builder as MigrationBuilder
participant ActionHandler as CreateViewSort<br/>ActionHandler
participant Cache as FlatEntityMaps<br/>Cache
participant DB as PostgreSQL
Client->>Resolver: createCoreViewSort(input)
Resolver->>Service: createOne({createViewSortInput, workspaceId})
Service->>Service: createMany([createViewSortInput])
Note over Service: Convert input to flat entity
Service->>Service: fromCreateViewSortInputTo<br/>FlatViewSortToCreate
Service->>Migration: validateBuildAndRun<br/>WorkspaceMigration
Migration->>Validator: validateFlatViewSortCreation
Note over Validator: Check duplicates,<br/>validate references
Validator->>Validator: Check viewSort doesn't exist
Validator->>Validator: Validate fieldMetadata exists
Validator->>Validator: Validate view exists
Validator->>Validator: Check no duplicate for<br/>viewId+fieldMetadataId
alt Validation fails
Validator-->>Migration: Validation errors
Migration-->>Service: WorkspaceMigration<br/>BuilderException
Service-->>Resolver: Exception
Resolver-->>Client: Error response
else Validation succeeds
Validator-->>Migration: Success
Migration->>Builder: Build migration actions
Builder-->>Migration: CreateViewSortAction
Migration->>ActionHandler: executeForMetadata
ActionHandler->>DB: INSERT viewSort
DB-->>ActionHandler: Success
ActionHandler-->>Migration: Success
Migration->>Cache: Invalidate cache
Cache->>DB: Recompute flatViewSortMaps
DB-->>Cache: ViewSort entities
Cache-->>Migration: Updated cache
Migration-->>Service: Success
Service->>Cache: getOrRecompute<br/>FlatViewSortMaps
Cache-->>Service: flatViewSortMaps
Service->>Service: findFlatEntityById<br/>InFlatEntityMaps
Service->>Service: fromFlatViewSortTo<br/>ViewSortDto
Service-->>Resolver: ViewSortDTO
Resolver-->>Client: Created ViewSort
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
packages/twenty-server/src/engine/metadata-modules/view-sort/resolvers/view-sort.resolver.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/metadata-modules/view-sort/resolvers/view-sort.resolver.ts
Outdated
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 65 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/engine/metadata-modules/view-sort/resolvers/view-sort.resolver.ts">
<violation number="1" location="packages/twenty-server/src/engine/metadata-modules/view-sort/resolvers/view-sort.resolver.ts:63">
P2: deleteCoreViewSort exposes ViewSortDTO in the GraphQL schema but returns a boolean, causing a schema/runtime mismatch.</violation>
<violation number="2" location="packages/twenty-server/src/engine/metadata-modules/view-sort/resolvers/view-sort.resolver.ts:63">
P2: createManyCoreViewSorts returns ViewSortDTO[] but the GraphQL decorator declares a single ViewSortDTO, causing a schema/return type mismatch. Declare a list return type in the mutation decorator.</violation>
</file>
<file name="packages/twenty-server/src/engine/workspace-manager/workspace-migration/services/workspace-migration-build-orchestrator.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/workspace-manager/workspace-migration/services/workspace-migration-build-orchestrator.service.ts:500">
P2: View sort actions are built and stored but never added to workspaceMigration.actions, so view sort changes will be ignored during migration.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/twenty-server/src/engine/metadata-modules/view-sort/resolvers/view-sort.resolver.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/metadata-modules/view-sort/resolvers/view-sort.resolver.ts
Outdated
Show resolved
Hide resolved
...space-manager/workspace-migration/services/workspace-migration-build-orchestrator.service.ts
Show resolved
Hide resolved
Contributor
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:22360 This environment will automatically shut down when the PR is closed or after 5 hours. |
packages/twenty-server/src/engine/metadata-modules/view-sort/resolvers/view-sort.resolver.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
3 issues found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/engine/metadata-modules/view-sort/resolvers/view-sort.resolver.ts">
<violation number="1" location="packages/twenty-server/src/engine/metadata-modules/view-sort/resolvers/view-sort.resolver.ts:108">
P2: Missing await on destroyOne means isDefined checks a Promise instead of the resolved DTO, returning true before deletion completes or fails and masking errors.</violation>
</file>
<file name="packages/twenty-front/src/modules/views/graphql/mutations/destroyCoreViewSort.ts">
<violation number="1" location="packages/twenty-front/src/modules/views/graphql/mutations/destroyCoreViewSort.ts:4">
P2: Mutation now requires `$input`, but existing caller still passes `{ id }`, which will cause GraphQL validation errors. Update callers to provide `input` matching `DestroyViewSortInput`.</violation>
</file>
<file name="packages/twenty-front/src/modules/views/graphql/mutations/updateCoreViewSort.ts">
<violation number="1" location="packages/twenty-front/src/modules/views/graphql/mutations/updateCoreViewSort.ts:6">
P2: Mutation definition removed the `id` argument, but call sites still send `id` and the server input requires it, causing GraphQL validation/runtime errors.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/twenty-server/src/engine/metadata-modules/view-sort/resolvers/view-sort.resolver.ts
Outdated
Show resolved
Hide resolved
| export const DESTROY_CORE_VIEW_SORT = gql` | ||
| mutation DestroyCoreViewSort($id: String!) { | ||
| destroyCoreViewSort(id: $id) | ||
| mutation DestroyCoreViewSort($input: DestroyViewSortInput!) { |
Contributor
There was a problem hiding this comment.
P2: Mutation now requires $input, but existing caller still passes { id }, which will cause GraphQL validation errors. Update callers to provide input matching DestroyViewSortInput.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/views/graphql/mutations/destroyCoreViewSort.ts, line 4:
<comment>Mutation now requires `$input`, but existing caller still passes `{ id }`, which will cause GraphQL validation errors. Update callers to provide `input` matching `DestroyViewSortInput`.</comment>
<file context>
@@ -1,7 +1,7 @@
export const DESTROY_CORE_VIEW_SORT = gql`
- mutation DestroyCoreViewSort($id: String!) {
- destroyCoreViewSort(id: $id)
+ mutation DestroyCoreViewSort($input: DestroyViewSortInput!) {
+ destroyCoreViewSort(input: $input)
}
</file context>
| ${VIEW_SORT_FRAGMENT} | ||
| mutation UpdateCoreViewSort($id: String!, $input: UpdateViewSortInput!) { | ||
| updateCoreViewSort(id: $id, input: $input) { | ||
| mutation UpdateCoreViewSort($input: UpdateViewSortInput!) { |
Contributor
There was a problem hiding this comment.
P2: Mutation definition removed the id argument, but call sites still send id and the server input requires it, causing GraphQL validation/runtime errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/views/graphql/mutations/updateCoreViewSort.ts, line 6:
<comment>Mutation definition removed the `id` argument, but call sites still send `id` and the server input requires it, causing GraphQL validation/runtime errors.</comment>
<file context>
@@ -3,8 +3,8 @@ import { gql } from '@apollo/client';
${VIEW_SORT_FRAGMENT}
- mutation UpdateCoreViewSort($id: String!, $input: UpdateViewSortInput!) {
- updateCoreViewSort(id: $id, input: $input) {
+ mutation UpdateCoreViewSort($input: UpdateViewSortInput!) {
+ updateCoreViewSort(input: $input) {
...ViewSortFragment
</file context>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors “view sort” to the v2 metadata/migration pipeline, wiring it into flat-entity maps, workspace migrations, and updated GraphQL inputs, with new integration coverage.
Changes:
- Add
viewSortas a first-class metadata entity (flat maps, relations, validators, migration builders/runners, cache keys). - Refactor
ViewSortServiceto create/update/delete/destroy via workspace-migration validate/build/run + flat-map recomputation. - Update front-end GraphQL operations/types and add server integration tests for view-sort create/update/delete/destroy.
Reviewed changes
Copilot reviewed 67 out of 68 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/twenty-shared/src/metadata/all-metadata-name.constant.ts | Adds viewSort to shared metadata name list. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/utils/update-one-core-view-sort.util.ts | Adds integration helper for update mutation calls. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/utils/update-core-view-sort-query-factory.util.ts | Adds update mutation query factory. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/utils/find-core-view-sorts.util.ts | Adds integration helper for listing view sorts. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/utils/find-core-view-sorts-query-factory.util.ts | Adds query factory for listing view sorts. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/utils/destroy-one-core-view-sort.util.ts | Adds integration helper for destroy mutation calls. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/utils/destroy-core-view-sort-query-factory.util.ts | Adds destroy mutation query factory. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/utils/delete-one-core-view-sort.util.ts | Adds integration helper for delete mutation calls. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/utils/delete-core-view-sort-query-factory.util.ts | Adds delete mutation query factory. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/utils/create-one-core-view-sort.util.ts | Adds integration helper for create mutation calls. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/utils/create-core-view-sort-query-factory.util.ts | Adds create mutation query factory. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/successful-view-sort-update.integration-spec.ts | Adds “successful update” integration tests. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/successful-view-sort-deletion.integration-spec.ts | Adds “successful deletion/destroy” integration tests. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/successful-view-sort-creation.integration-spec.ts | Adds “successful create” integration tests. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/failing-view-sort-update.integration-spec.ts | Adds “failing update” integration tests. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/failing-view-sort-deletion.integration-spec.ts | Adds “failing delete/destroy” integration tests. |
| packages/twenty-server/test/integration/metadata/suites/view-sort/failing-view-sort-creation.integration-spec.ts | Adds “failing create” integration tests. |
| packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/utils/optimistically-apply-update-action-on-all-flat-entity-maps.util.ts | Enables optimistic update handling for viewSort. |
| packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/utils/optimistically-apply-delete-action-on-all-flat-entity-maps.util.ts | Enables optimistic delete handling for viewSort. |
| packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/utils/optimistically-apply-create-action-on-all-flat-entity-maps.util.ts | Enables optimistic create handling for viewSort. |
| packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/workspace-schema-migration-runner-action-handlers.module.ts | Registers view-sort migration action handlers. |
| packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/view-sort/services/update-view-sort-action-handler.service.ts | Implements migration “update viewSort” runner. |
| packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/view-sort/services/delete-view-sort-action-handler.service.ts | Implements migration “delete viewSort” runner. |
| packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/view-sort/services/create-view-sort-action-handler.service.ts | Implements migration “create viewSort” runner. |
| packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/workspace-migration-builder.module.ts | Registers view-sort actions builder. |
| packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/validators/workspace-migration-builder-validators.module.ts | Registers view-sort validator service. |
| packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/validators/services/flat-view-sort-validator.service.ts | Adds flat-entity validation rules for view sorts. |
| packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/builders/view-sort/workspace-migration-view-sort-actions.builder.service.ts | Adds builder that validates/builds view-sort actions. |
| packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/builders/view-sort/types/workspace-migration-view-sort-action.type.ts | Adds typed migration actions for view sort. |
| packages/twenty-server/src/engine/workspace-manager/workspace-migration/services/workspace-migration-build-orchestrator.service.ts | Orchestrates view-sort actions within workspace migration builds. |
| packages/twenty-server/src/engine/workspace-manager/twenty-standard-application/utils/view/create-standard-view-flat-metadata.util.ts | Initializes viewSortIds on standard flat views. |
| packages/twenty-server/src/engine/workspace-manager/twenty-standard-application/utils/twenty-standard-application-all-flat-entity-maps.constant.ts | Creates empty flatViewSortMaps (currently unused). |
| packages/twenty-server/src/engine/workspace-cache/types/workspace-cache-key.type.ts | Adds cache key for flatViewSortMaps. |
| packages/twenty-server/src/engine/metadata-modules/view-sort/view-sort.module.ts | Wires additional modules needed for v2 view-sort flow. |
| packages/twenty-server/src/engine/metadata-modules/view-sort/utils/from-flat-view-sort-to-view-sort-dto.util.ts | Adds conversion from flat view-sort to DTO. |
| packages/twenty-server/src/engine/metadata-modules/view-sort/services/view-sort.service.ts | Refactors ViewSortService to migration/flat-map-based CRUD. |
| packages/twenty-server/src/engine/metadata-modules/view-sort/services/tests/view-sort.service.spec.ts | Removes unit tests for ViewSortService. |
| packages/twenty-server/src/engine/metadata-modules/view-sort/resolvers/view-sort.resolver.ts | Updates GraphQL resolver to new input patterns + createMany. |
| packages/twenty-server/src/engine/metadata-modules/view-sort/dtos/inputs/update-view-sort.input.ts | Redefines update input as { id, update } with validation. |
| packages/twenty-server/src/engine/metadata-modules/view-sort/dtos/inputs/destroy-view-sort.input.ts | Adds destroy input DTO. |
| packages/twenty-server/src/engine/metadata-modules/view-sort/dtos/inputs/delete-view-sort.input.ts | Adds delete input DTO. |
| packages/twenty-server/src/engine/metadata-modules/view-sort/controllers/view-sort.controller.ts | Updates REST endpoints to use v2 service methods/inputs. |
| packages/twenty-server/src/engine/metadata-modules/flat-view/utils/from-view-entity-to-flat-view.util.ts | Includes viewSort ids/universal identifiers in flat view projection. |
| packages/twenty-server/src/engine/metadata-modules/flat-view/utils/from-create-view-input-to-flat-view-to-create.util.ts | Initializes viewSortIds for created views. |
| packages/twenty-server/src/engine/metadata-modules/flat-view/types/flat-view.type.ts | Removes temporary omission of viewSorts relation from FlatView type. |
| packages/twenty-server/src/engine/metadata-modules/flat-view/services/workspace-flat-view-map-cache.service.ts | Includes viewSorts in flat view cache computation. |
| packages/twenty-server/src/engine/metadata-modules/flat-view/flat-view.module.ts | Registers ViewSortEntity for flat-view module queries. |
| packages/twenty-server/src/engine/metadata-modules/flat-view-sort/utils/from-view-sort-entity-to-flat-view-sort.util.ts | Adds entity→flat conversion for view sorts. |
| packages/twenty-server/src/engine/metadata-modules/flat-view-sort/utils/from-update-view-sort-input-to-flat-view-sort-to-update-or-throw.util.ts | Adds update input→flat update conversion. |
| packages/twenty-server/src/engine/metadata-modules/flat-view-sort/utils/from-destroy-view-sort-input-to-flat-view-sort-or-throw.util.ts | Adds destroy input→flat lookup conversion. |
| packages/twenty-server/src/engine/metadata-modules/flat-view-sort/utils/from-delete-view-sort-input-to-flat-view-sort-or-throw.util.ts | Adds delete input→flat soft-delete conversion. |
| packages/twenty-server/src/engine/metadata-modules/flat-view-sort/utils/from-create-view-sort-input-to-flat-view-sort-to-create.util.ts | Adds create input→flat create conversion. |
| packages/twenty-server/src/engine/metadata-modules/flat-view-sort/types/flat-view-sort.type.ts | Defines FlatViewSort type. |
| packages/twenty-server/src/engine/metadata-modules/flat-view-sort/types/flat-view-sort-maps.type.ts | Defines FlatViewSortMaps type. |
| packages/twenty-server/src/engine/metadata-modules/flat-view-sort/services/workspace-flat-view-sort-map-cache.service.ts | Adds cache provider for flat view-sort maps. |
| packages/twenty-server/src/engine/metadata-modules/flat-view-sort/constants/flat-view-sort-editable-properties.constant.ts | Declares editable properties for view sorts. |
| packages/twenty-server/src/engine/metadata-modules/flat-entity/types/all-flat-entity-types-by-metadata-name.ts | Adds viewSort to global flat-entity type mapping. |
| packages/twenty-server/src/engine/metadata-modules/flat-entity/services/workspace-many-or-all-flat-entity-maps-cache.module.ts | Registers ViewSortEntity + view-sort cache service. |
| packages/twenty-server/src/engine/metadata-modules/flat-entity/constant/all-metadata-required-metadata-for-validation.constant.ts | Requires view + fieldMetadata for view-sort validation. |
| packages/twenty-server/src/engine/metadata-modules/flat-entity/constant/all-metadata-relations.constant.ts | Adds viewSort relations and wires view→viewSort aggregator. |
| packages/twenty-server/src/engine/metadata-modules/flat-entity/constant/all-metadata-entity-by-metadata-name.constant.ts | Registers ViewSortEntity by metadata name. |
| packages/twenty-server/src/engine/metadata-modules/flat-entity/constant/all-flat-entity-properties-to-compare-and-stringify.constant.ts | Adds viewSort properties for diffing/comparison. |
| packages/twenty-front/src/modules/views/graphql/mutations/updateCoreViewSort.ts | Updates update mutation signature to single input. |
| packages/twenty-front/src/modules/views/graphql/mutations/destroyCoreViewSort.ts | Updates destroy mutation to accept input. |
| packages/twenty-front/src/modules/views/graphql/mutations/deleteCoreViewSort.ts | Updates delete mutation to accept input. |
| packages/twenty-front/src/modules/metadata-error-handler/hooks/useMetadataErrorHandler.ts | Adds viewSort label for metadata error handling. |
| packages/twenty-front/src/generated/graphql.ts | Updates generated schema/types for viewSort inputs + mutation args. |
| packages/twenty-front/src/generated-metadata/graphql.ts | Updates generated metadata schema/types for viewSort inputs + mutation args. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
104
to
114
| async destroyCoreViewSort( | ||
| @Args('id', { type: () => String }) id: string, | ||
| @AuthWorkspace() workspace: WorkspaceEntity, | ||
| @Args('input') destroyViewSortInput: DestroyViewSortInput, | ||
| @AuthWorkspace() { id: workspaceId }: WorkspaceEntity, | ||
| ): Promise<boolean> { | ||
| return this.viewSortService.destroy(id, workspace.id); | ||
| const destroyedViewSort = this.viewSortService.destroyOne({ | ||
| destroyViewSortInput, | ||
| workspaceId, | ||
| }); | ||
|
|
||
| return isDefined(destroyedViewSort); | ||
| } |
There was a problem hiding this comment.
destroyCoreViewSort doesn’t await the service call. As written, destroyedViewSort is a Promise so isDefined(destroyedViewSort) will always be true, even if the operation fails later. Await the call (and/or return the awaited boolean/DTO directly) so the mutation result reflects the actual outcome.
Comment on lines
+23
to
+36
| const createdAt = new Date().toISOString(); | ||
| const viewSortId = createViewSortInput.id ?? v4(); | ||
|
|
||
| return { | ||
| id: viewSortId, | ||
| viewId, | ||
| fieldMetadataId, | ||
| workspaceId, | ||
| createdAt, | ||
| updatedAt: createdAt, | ||
| deletedAt: null, | ||
| universalIdentifier: viewSortId, | ||
| direction: createViewSortInput.direction ?? ViewSortDirection.ASC, | ||
| applicationId: workspaceCustomApplicationId, |
There was a problem hiding this comment.
universalIdentifier is being set to viewSortId (the entity id). Other flat-metadata create helpers generate a separate universal identifier (e.g. fromCreateViewGroupInputToFlatViewGroupToCreate sets universalIdentifier: ... ?? v4()). Using id here changes semantics compared to existing data created by the old service (which generated universalIdentifier independently) and can break workflows relying on stable universal identifiers. Generate a new UUID for universalIdentifier (or follow the project’s standardId/universalIdentifier convention) instead of reusing id.
Comment on lines
17
to
19
| }): CommonResponseBody<{ | ||
| deleteCoreViewSort: ViewSortDTO; | ||
| }> => { |
There was a problem hiding this comment.
This helper types deleteCoreViewSort as returning ViewSortDTO, but the server resolver/front-end operations indicate deleteCoreViewSort returns a Boolean. Once the mutation document is fixed (no selection set), update this response typing (and downstream assertions) to match the actual schema return type.
Comment on lines
17
to
19
| }): CommonResponseBody<{ | ||
| destroyCoreViewSort: ViewSortDTO; | ||
| }> => { |
There was a problem hiding this comment.
This helper expects destroyCoreViewSort to return a ViewSortDTO, but the resolver/front-end mutation indicate destroyCoreViewSort returns Boolean. After fixing the mutation document (no selection set), update this response typing and any assertions accordingly (or change the server contract to return a DTO).
Comment on lines
104
to
111
| const { data: deleteData } = await deleteOneCoreViewSort({ | ||
| expectToFail: false, | ||
| viewSortId, | ||
| }); | ||
|
|
||
| expect(deleteData.deleteCoreViewSort).toMatchObject({ | ||
| id: viewSortId, | ||
| }); |
There was a problem hiding this comment.
This test asserts deleteCoreViewSort returns a DTO with id, but the deleteCoreViewSort mutation is defined as returning a Boolean in the resolver/front-end operations. Either change the mutation contract to return ViewSortDTO (like other metadata modules), or update this test to assert the boolean return value and keep validating deletion via the subsequent getCoreViewSorts call.
Comment on lines
89
to
92
| const flatViewFilterGroupMaps = createEmptyFlatEntityMaps(); | ||
|
|
||
| const flatViewSortMaps = createEmptyFlatEntityMaps(); | ||
|
|
There was a problem hiding this comment.
flatViewSortMaps is created but never used or returned. This will trigger noUnusedLocals/lint failures and is dead code. Either remove it, or add viewSort to TWENTY_STANDARD_ALL_METADATA_NAME and include flatViewSortMaps in the returned TwentyStandardAllFlatEntityMaps if standard apps should expose an empty map.
Comment on lines
11
to
17
| query: gql` | ||
| mutation DeleteCoreViewSort($input: DeleteViewSortInput!) { | ||
| deleteCoreViewSort(input: $input) { | ||
| ${gqlFields} | ||
| } | ||
| } | ||
| `, |
There was a problem hiding this comment.
deleteCoreViewSort is defined as returning a scalar (front-end operation and resolver both treat it as Boolean), but this query factory requests a selection set ({ ${gqlFields} }). That makes the GraphQL operation invalid. Remove the selection set and update callers/return types to expect a boolean (or change the server mutation to return ViewSortDTO consistently with other metadata modules).
Comment on lines
11
to
17
| query: gql` | ||
| mutation DestroyCoreViewSort($input: DestroyViewSortInput!) { | ||
| destroyCoreViewSort(input: $input) { | ||
| ${gqlFields} | ||
| } | ||
| } | ||
| `, |
There was a problem hiding this comment.
destroyCoreViewSort is treated as returning a scalar (Boolean) in the resolver/front-end mutation, but this query factory requests fields ({ ${gqlFields} }). That GraphQL document won’t validate. Remove the selection set and adjust the expected response type (or update the server mutation to return ViewSortDTO if that’s the intended contract).
Comment on lines
144
to
151
| const { data: destroyData } = await destroyOneCoreViewSort({ | ||
| expectToFail: false, | ||
| viewSortId, | ||
| }); | ||
|
|
||
| expect(destroyData.destroyCoreViewSort).toMatchObject({ | ||
| id: viewSortId, | ||
| }); |
There was a problem hiding this comment.
This test asserts destroyCoreViewSort returns a DTO with id, but the destroyCoreViewSort mutation is defined as returning a Boolean in the resolver/front-end operations. Align the test with the schema (assert boolean) or change the server mutation to return ViewSortDTO if that’s the intended API.
prastoin
reviewed
Feb 2, 2026
| id: viewSort.id, | ||
| input: { | ||
| id: viewSort.id, | ||
| direction: viewSort.direction, |
Contributor
There was a problem hiding this comment.
Suggestion: We should not store the id of the updated entity at the same lvl as the other properties, as id in the end isn't mutable
We should add an updates sub record
Contributor
There was a problem hiding this comment.
Bug: That's broken regarding the contract API, one more reason to refactor using strictly typed codegen
export class UpdateViewSortInput {
@IsUUID()
@IsNotEmpty()
@Field(() => UUIDScalarType, {
description: 'The id of the view sort to update',
})
id: string;
@Type(() => UpdateViewSortInputUpdates)
@ValidateNested()
@Field(() => UpdateViewSortInputUpdates, {
description: 'The view sort to update',
})
update: UpdateViewSortInputUpdates;
}| @@ -57,8 +57,8 @@ export const usePerformViewSortAPIPersist = () => { | |||
| apolloClient.mutate({ | |||
| mutation: UPDATE_CORE_VIEW_SORT, | |||
Contributor
There was a problem hiding this comment.
Remark: Unless I'm mistaken this is not strictly typed
We refactor the whole service to be consuming the auto-generated mutations hooks such as useCreateCoreViewSortMutation following the same pattern as what's done in packages/twenty-front/src/modules/views/hooks/internal/usePerformViewFilterAPIPersist.ts
|
|
||
| import { UUIDScalarType } from 'src/engine/api/graphql/workspace-schema-builder/graphql-types/scalars'; | ||
| import { ViewSortDirection } from 'src/engine/metadata-modules/view-sort/enums/view-sort-direction'; | ||
|
|
Contributor
There was a problem hiding this comment.
Request: Please also pass over the createViewSortInput adding some class validators decorators too
| ); | ||
|
|
||
| const savedViewSort = await this.viewSortRepository.save(viewSort); | ||
| return fromFlatViewSortToViewSortDto( |
Contributor
There was a problem hiding this comment.
Suggestion: Services should always return FlatEntity and resolvers should transpile them to Dto
| @AuthWorkspace() workspace: WorkspaceEntity, | ||
| @Args('input') deleteViewSortInput: DeleteViewSortInput, | ||
| @AuthWorkspace() { id: workspaceId }: WorkspaceEntity, | ||
| ): Promise<boolean> { |
Contributor
There was a problem hiding this comment.
Polish: not required but we should rather return the whole updated entry
| } | ||
|
|
||
| await this.viewSortRepository.softDelete(id); | ||
| return fromFlatViewSortToViewSortDto(existingViewSortToDelete); |
Contributor
There was a problem hiding this comment.
Praise: Nice ! ( should not transpile dto here as said above )
|
|
||
| const flatViewFilterGroupMaps = createEmptyFlatEntityMaps(); | ||
|
|
||
| const flatViewSortMaps = createEmptyFlatEntityMaps(); |
Contributor
There was a problem hiding this comment.
Polish: unused and won't be used as there's no twenty standard view sort entities
...space-manager/workspace-migration/services/workspace-migration-build-orchestrator.service.ts
Show resolved
Hide resolved
| input: { | ||
| id: v4(), | ||
| update: { | ||
| direction: ViewSortDirection.DESC, |
Contributor
There was a problem hiding this comment.
remark: add invalid direction use case
Comment on lines
194
to
195
| return validationResult; | ||
| } |
Contributor
There was a problem hiding this comment.
Request: in both create and udpate we should verify that if updated direction is valid enum member
...igration/workspace-migration-builder/validators/services/flat-view-sort-validator.service.ts
Show resolved
Hide resolved
packages/twenty-front/src/modules/views/hooks/useSaveRecordSortsToViewSorts.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
2 issues found across 19 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/validators/services/flat-view-sort-validator.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/validators/services/flat-view-sort-validator.service.ts:101">
P2: Creation validation only flags invalid direction when it is undefined because the condition uses `&&`; defined but invalid direction values pass validation. Use `||` to reject undefined OR invalid enum values.</violation>
<violation number="2" location="packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/validators/services/flat-view-sort-validator.service.ts:155">
P2: Update validation skips invalid direction values because the condition only triggers when direction is undefined and `(ASC || DESC)` collapses to `ASC`. This lets invalid directions pass in updates.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
...igration/workspace-migration-builder/validators/services/flat-view-sort-validator.service.ts
Outdated
Show resolved
Hide resolved
...igration/workspace-migration-builder/validators/services/flat-view-sort-validator.service.ts
Outdated
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes twentyhq/core-team-issues#2187