Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if (parameters.calendarFieldName) { | ||
| calendarFieldMetadataId = await this.resolveFieldMetadataId( | ||
| workspaceId, | ||
| objectMetadataId, | ||
| parameters.calendarFieldName, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: The tool for creating calendar views doesn't validate that calendarFieldName is a DATE or DATE_TIME field, only that the field exists.
Severity: MEDIUM
Suggested Fix
In view-tools.factory.ts, after resolving the calendarFieldMetadataId, add a check to ensure the corresponding field's type is DATE or DATE_TIME. You can use the existing isFieldMetadataDateKind utility for this validation and throw an error if the type is incorrect, similar to how group-by fields are handled for Kanban views.
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-server/src/engine/metadata-modules/view/tools/view-tools.factory.ts#L329-L335
Potential issue: The code at `view-tools.factory.ts` resolves the `calendarFieldName`
using the generic `resolveFieldMetadataId`, which only validates the field's existence.
This is inconsistent with the new `resolveGroupByFieldMetadataId` function that
correctly validates the field type for Kanban views. While the Zod schema description
and frontend code both specify that a date-type field is required, this is not enforced
at the tool layer. This allows for the creation of a misconfigured calendar view if a
non-date field is provided, which will cause the calendar to malfunction or display no
data.
Did we get this right? 👍 / 👎 to inform future reviews.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 23 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-server/src/engine/metadata-modules/view/tools/view-tools.factory.ts">
<violation number="1" location="packages/twenty-server/src/engine/metadata-modules/view/tools/view-tools.factory.ts:330">
P2: Validate that calendarFieldName refers to a DATE/DATE_TIME field before creating the view. Right now any field type passes, which can create invalid calendar views.</violation>
</file>
<file name="packages/twenty-server/src/engine/workspace-manager/twenty-standard-application/utils/skill-metadata/create-standard-flat-skill-metadata.util.ts">
<violation number="1" location="packages/twenty-server/src/engine/workspace-manager/twenty-standard-application/utils/skill-metadata/create-standard-flat-skill-metadata.util.ts:230">
P2: The skill instructions refer to a "navigate" tool, but the only registered navigation tool is `navigate_app`. This will prompt invalid tool calls when the skill is executed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| if (parameters.calendarFieldName) { | ||
| calendarFieldMetadataId = await this.resolveFieldMetadataId( |
There was a problem hiding this comment.
P2: Validate that calendarFieldName refers to a DATE/DATE_TIME field before creating the view. Right now any field type passes, which can create invalid calendar views.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/metadata-modules/view/tools/view-tools.factory.ts, line 330:
<comment>Validate that calendarFieldName refers to a DATE/DATE_TIME field before creating the view. Right now any field type passes, which can create invalid calendar views.</comment>
<file context>
@@ -257,6 +326,14 @@ export class ViewToolsFactory {
}
+ if (parameters.calendarFieldName) {
+ calendarFieldMetadataId = await this.resolveFieldMetadataId(
+ workspaceId,
+ objectMetadataId,
</file context>
|
|
||
| STEP 7.5: Add view fields to the default views of standard objects to expose the new custom fields. | ||
| For each of People, Companies, and Opportunities: | ||
| - Navigate to the object's default view using the navigate tool |
There was a problem hiding this comment.
P2: The skill instructions refer to a "navigate" tool, but the only registered navigation tool is navigate_app. This will prompt invalid tool calls when the skill is executed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/workspace-manager/twenty-standard-application/utils/skill-metadata/create-standard-flat-skill-metadata.util.ts, line 230:
<comment>The skill instructions refer to a "navigate" tool, but the only registered navigation tool is `navigate_app`. This will prompt invalid tool calls when the skill is executed.</comment>
<file context>
@@ -193,25 +214,100 @@ targetFieldIcon is like IconSomething, it's ok if it doesn't exist in the icon l
+
+STEP 7.5: Add view fields to the default views of standard objects to expose the new custom fields.
+For each of People, Companies, and Opportunities:
+- Navigate to the object's default view using the navigate tool
+- Wait 3 seconds
+- Use create_many_view_fields to add all the new custom fields to the default view so they are visible
</file context>
| - Navigate to the object's default view using the navigate tool | |
| - Navigate to the object's default view using the navigate_app tool |
There was a problem hiding this comment.
Pull request overview
This PR enhances the demo-workspace creation “skill” guidance and expands dashboard/view tooling to support building richer demo dashboards (including a new RECORD_TABLE widget) and more complete view configuration (fields, filters, sorts), while also tightening some backend cache/menu cleanup behaviors and improving agent-chat token retry logic.
Changes:
- Add support for a
RECORD_TABLEdashboard widget configuration + update dashboard tool docs to require dedicated TABLE views for widgets. - Extend view tooling: enhance
create_viewinputs (calendar settings, optionalfieldNames) and add new View Filter/Sort tool factories + providers. - Refine standard skill metadata (workspace demo seeding + new “view-*” and cleanup skills) and improve navigation menu cleanup on object deactivation.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/twenty-server/src/modules/dashboard/tools/schemas/widget.schema.ts | Adds RECORD_TABLE widget type and configuration schema (viewId-based). |
| packages/twenty-server/src/modules/dashboard/tools/create-complete-dashboard.tool.ts | Updates dashboard widget documentation/requirements for RECORD_TABLE. |
| packages/twenty-server/src/modules/dashboard/tools/add-dashboard-widget.tool.ts | Documents RECORD_TABLE widget workflow (dedicated view prerequisite). |
| packages/twenty-server/src/engine/workspace-manager/twenty-standard-application/utils/skill-metadata/create-standard-flat-skill-metadata.util.ts | Major prompt/content refinement for demo seeding + adds view-building/filtering/cleanup skill content. |
| packages/twenty-server/src/engine/workspace-manager/twenty-standard-application/constants/standard-skill.constant.ts | Registers new standard skills with universal identifiers. |
| packages/twenty-server/src/engine/metadata-modules/view/view.module.ts | Wires ViewFieldModule + ViewFilterModule into ViewModule. |
| packages/twenty-server/src/engine/metadata-modules/view/tools/view-tools.factory.ts | Enhances create_view (calendar params, fieldNames) + adds group-by SELECT validation. |
| packages/twenty-server/src/engine/metadata-modules/view/tools/tests/view-tools.factory.spec.ts | Updates tests for new create_view behavior (needs fixture alignment). |
| packages/twenty-server/src/engine/metadata-modules/view-sort/view-sort.module.ts | Exposes ViewSortToolsFactory via module providers/exports. |
| packages/twenty-server/src/engine/metadata-modules/view-sort/tools/view-sort-tools.factory.ts | Introduces tool factory for view sort CRUD operations. |
| packages/twenty-server/src/engine/metadata-modules/view-filter/view-filter.module.ts | Exposes ViewFilterToolsFactory via module providers/exports. |
| packages/twenty-server/src/engine/metadata-modules/view-filter/tools/view-filter-tools.factory.ts | Introduces tool factory for view filter CRUD operations. |
| packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts | Invalidates navigation menu item flat maps when object isActive changes. |
| packages/twenty-server/src/engine/metadata-modules/navigation-menu-item/services/navigation-menu-item-deletion.service.ts | Adds deletion of nav menu items that target deactivated objects. |
| packages/twenty-server/src/engine/metadata-modules/navigation-menu-item/navigation-menu-item.module.ts | Registers new object-deactivation listener. |
| packages/twenty-server/src/engine/metadata-modules/navigation-menu-item/listeners/navigation-menu-item-object-deactivation.listener.ts | Deletes nav menu items when objects are deactivated via metadata events. |
| packages/twenty-server/src/engine/metadata-modules/ai/ai-chat/services/system-prompt-builder.service.ts | Adds labels for new VIEW_FILTER/VIEW_SORT tool categories. |
| packages/twenty-server/src/engine/core-modules/tool/tools/navigate-tool/navigate-app-tool.ts | Improves object navigation matching using Fuse over labels + handles OBJECT vs VIEW nav items. |
| packages/twenty-server/src/engine/core-modules/tool-provider/tool-provider.module.ts | Registers new view filter/sort tool providers and modules. |
| packages/twenty-server/src/engine/core-modules/tool-provider/providers/view-sort-tool.provider.ts | New provider to expose view sort tools (permission-gated writes). |
| packages/twenty-server/src/engine/core-modules/tool-provider/providers/view-filter-tool.provider.ts | New provider to expose view filter tools (permission-gated writes). |
| packages/twenty-server/src/engine/core-modules/tool-provider/enums/tool-category.enum.ts | Adds VIEW_FILTER and VIEW_SORT categories. |
| packages/twenty-front/src/modules/ai/hooks/useAgentChat.ts | Improves 401 retry by reusing an already-updated token before attempting renewal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (parameters.mainGroupByFieldName) { | ||
| mainGroupByFieldMetadataId = await this.resolveFieldMetadataId( | ||
| workspaceId, | ||
| objectMetadataId, | ||
| parameters.mainGroupByFieldName, | ||
| ); | ||
| mainGroupByFieldMetadataId = | ||
| await this.resolveGroupByFieldMetadataId( | ||
| workspaceId, | ||
| objectMetadataId, | ||
| parameters.mainGroupByFieldName, | ||
| ); | ||
| } |
There was a problem hiding this comment.
create_view allows creating a KANBAN view without mainGroupByFieldName even though the schema/description says it's required. That can lead to invalid/unusable board views being created. Consider enforcing mainGroupByFieldName when type === ViewType.KANBAN (and returning a clear error before calling viewService.createOne).
| @@ -268,11 +345,38 @@ export class ViewToolsFactory { | |||
| kanbanAggregateOperation: | |||
| parameters.kanbanAggregateOperation as AggregateOperations, | |||
| kanbanAggregateOperationFieldMetadataId, | |||
| calendarLayout: parameters.calendarLayout, | |||
| calendarFieldMetadataId, | |||
| }, | |||
There was a problem hiding this comment.
create_view documents calendarFieldName + calendarLayout as required for CALENDAR views, and the ViewEntity DB constraint (CHK_VIEW_CALENDAR_INTEGRITY) will reject CALENDAR rows with either missing. Right now the tool passes both through as optional, which will surface as a low-signal migration/DB error. Suggest: if type === ViewType.CALENDAR, require both parameters and validate that the resolved field metadata type is DATE or DATE_TIME before creating the view.
| const mockFlatFieldMetadataMaps = { | ||
| byUniversalIdentifier: { | ||
| 'field-universal-id': { | ||
| id: mockCalendarFieldMetadataId, | ||
| name: 'dueAt', | ||
| objectMetadataId: mockObjectMetadataId, | ||
| universalIdentifier: 'field-universal-id', | ||
| }, | ||
| 'name-field-universal-id': { | ||
| id: mockNameFieldMetadataId, | ||
| name: 'name', | ||
| objectMetadataId: mockObjectMetadataId, | ||
| universalIdentifier: 'name-field-universal-id', | ||
| }, | ||
| 'stage-field-universal-id': { | ||
| id: mockStageFieldMetadataId, | ||
| name: 'stage', | ||
| objectMetadataId: mockObjectMetadataId, | ||
| universalIdentifier: 'stage-field-universal-id', | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The new group-by validation in resolveGroupByFieldMetadataId checks fieldMetadata.type === FieldMetadataType.SELECT, but the test fixtures in mockFlatFieldMetadataMaps don't include a type property. As written, create_view with mainGroupByFieldName: 'stage' will now throw in tests. Update the mocked field metadata to include type (e.g. stage as SELECT, dueAt as DATE/DATE_TIME).
| const filter = await this.viewFilterService.createOne({ | ||
| createViewFilterInput: { | ||
| viewId: parameters.viewId, | ||
| fieldMetadataId: parameters.fieldMetadataId, | ||
| operand: parameters.operand, | ||
| value: parameters.value as string, | ||
| subFieldName: parameters.subFieldName, | ||
| }, |
There was a problem hiding this comment.
In the view filter tools, value is asserted as string when building CreateViewFilterInput/UpdateViewFilterInput (parameters.value as string). CreateViewFilterInput.value is actually ViewFilterValue (jsonb) and supports strings, numbers, booleans, arrays, and objects. Using a string assertion here is misleading and weakens type-safety—prefer parameters.value as ViewFilterValue (or just parameters.value) consistently for create/update/batch paths.
| }; | ||
|
|
||
| @Injectable() | ||
| export class NavigationMenuItemObjectDeactivationListener { |
There was a problem hiding this comment.
Not sure if it's the kind of thing we want to do (with apps it will become even more complex!), it feels like cascading this kind of logic makes it hard to restore. Have we considered just hiding these navigation items when displaying them instead of doing a listener?
There was a problem hiding this comment.
Overall, the less event-based logic the better, it's cause for bugs
| import { ViewSortToolsFactory } from 'src/engine/metadata-modules/view-sort/tools/view-sort-tools.factory'; | ||
|
|
||
| @Injectable() | ||
| export class ViewSortToolProvider implements ToolProvider, OnModuleInit { |
There was a problem hiding this comment.
I think it was a mistake to do a a ToolProvider for View + ViewField, and now one for ViewSort. For me it should be a single View ToolProvider for all View-related tools
| import { ViewFilterToolsFactory } from 'src/engine/metadata-modules/view-filter/tools/view-filter-tools.factory'; | ||
|
|
||
| @Injectable() | ||
| export class ViewFilterToolProvider implements ToolProvider, OnModuleInit { |
There was a problem hiding this comment.
Same comment, I think it doesn't deserve a dedicated provider, we should put everything into one View ToolProvider imo
ViewFilterToolsFactory+ViewFilterToolProvider(new tool categoryVIEW_FILTER, permission-gated)ViewSortToolsFactory+ViewSortToolProvider(new tool categoryVIEW_SORT, permission-gated)create_viewnow acceptscalendarFieldName+fieldNamesto set view columns at creation time —ViewToolsFactory(injectsViewFieldService, creates view fields inline)view-building,view-filters-and-sorts,custom-objects-cleanup—standard-skill.constant.ts+create-standard-flat-skill-metadata.util.tsworkspace-demo-seedingskill reworked: keeps standard objects (People, Companies, Opportunities), adds custom fields and renames the first N records by position instead of creating fully custom objects — preserves existing emails and calendar events on standard objectsNavigationMenuItemObjectDeactivationListenerlistens tometadata.objectMetadata.updatedand deletes nav items whenisActiveflips to falseuseAgentChatbut this is still not stableTODO :
Still a bug on record table widget creation from the skill, seems like we should enforce the label identifier :