-
Notifications
You must be signed in to change notification settings - Fork 4.7k
feat(ai): add browsing context and fix tool loading #16476
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
- Add BrowsingContext type to pass what the user is currently viewing (recordPage or listView) to the AI chat - Simplify context architecture: remove toggleable context UI, make it automatic and invisible to the user - Fix tool loading: add unionOf handling and fix regex ordering so find_one_* tools are properly registered - Use plural names for find tools (find_people vs find_one_person) - Clean up unused components and states
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 20 files
Prompt for AI agents (all 1 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/core-modules/tool-provider/services/tool-registry.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/tool-provider/services/tool-registry.service.ts:515">
P1: The `unionOf` handling silently ignores all roles except the first one. Other parts of the codebase (workspace-entity-manager.ts, per-object-tool-generator.service.ts) explicitly check if `unionOf.length === 1` and throw an error for multiple roles. This code should do the same to avoid silently incorrect permission behavior.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| // For unionOf with single role, just use that role's permissions | ||
| objectPermissions = allRolePermissions[0]; |
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.
P1: The unionOf handling silently ignores all roles except the first one. Other parts of the codebase (workspace-entity-manager.ts, per-object-tool-generator.service.ts) explicitly check if unionOf.length === 1 and throw an error for multiple roles. This code should do the same to avoid silently incorrect permission behavior.
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/core-modules/tool-provider/services/tool-registry.service.ts, line 515:
<comment>The `unionOf` handling silently ignores all roles except the first one. Other parts of the codebase (workspace-entity-manager.ts, per-object-tool-generator.service.ts) explicitly check if `unionOf.length === 1` and throw an error for multiple roles. This code should do the same to avoid silently incorrect permission behavior.</comment>
<file context>
@@ -505,6 +507,13 @@ export class ToolRegistryService {
+ (roleId: string) => rolesPermissions[roleId],
+ );
+
+ // For unionOf with single role, just use that role's permissions
+ objectPermissions = allRolePermissions[0];
} else {
</file context>
| // For unionOf with single role, just use that role's permissions | |
| objectPermissions = allRolePermissions[0]; | |
| // For unionOf with single role, just use that role's permissions | |
| if (allRolePermissions.length === 1) { | |
| objectPermissions = allRolePermissions[0]; | |
| } else { | |
| throw new Error( | |
| 'Union permission logic for multiple roles not yet implemented', | |
| ); | |
| } |
✅ Addressed in 8f0df54
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:52824 This environment will automatically shut down when the PR is closed or after 5 hours. |
…ata tools - Introduce ToolProvider interface for consistent tool category handling - Create ActionToolProvider, DatabaseToolProvider, MetadataToolProvider, WorkflowToolProvider - Simplify ToolRegistryService from 702 to 185 lines using provider pattern - Register metadata tools (object/field CRUD) in AI chat - Rename search_articles to search_help_center for clarity
NestJS dependency injection requires regular imports (not 'import type') for injectable services. Using 'import type' causes the class reference to be unavailable at runtime, resulting in 'undefined' being injected.
|
Hey @FelixMalfait! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
Summary
BrowsingContexttype to automatically pass what the user is currently viewing (recordPage or listView) to the AI chatunionOfhandling ingetDatabaseToolsForObjectand fix regex ordering sofind_one_*tools are properly registeredfind_peoplevsfind_one_person) for better semanticsChanges
Frontend
BrowsingContexttype anduseGetBrowsingContexthook to gather context from Recoil stateuseAgentChatto use the new browsing contextAgentChatContextRecordPreview,SendMessageWithRecordsContextButton, etc.)isAgentChatCurrentContextActiveStateBackend
BrowsingContextTypefor recordPage and listView contextsChatExecutionServiceto build context from browsing contexttool-registry.service.ts:unionOfhandling in permission configfind_onebeforefind) so tools load correctlyfind_peopleinstead offind_person)Test plan
find_one_*tools now load correctlyfind_*tools use plural naming