WIP: Knowledge base BoK instead of Subspace in Written Knowledge creation #7381
Conversation
…for users + orgs + vcs are managed; moved some global admin functionality out of domain down to platform admin; ...
… types + ID passing for org mutations
…into server-4742
* VC documents and links BoK - refactor the AddContent * VC documents and links implementation without validation; * resolve rabbit comments --------- Co-authored-by: Petar Kolev <[email protected]> Co-authored-by: reactoholic <[email protected]>
* codegen passing with updated api * fixed api + codegen passes * code compiling * pick up create callout privilege from the CalloutsSet * callouts showing up after creation * retrieving of callouts using only calloutsSet ID * moved code around to have notion of calloutsSet in tree * fix array dep breaking tool creation; small code optimizations; --------- Co-authored-by: bobbykolev <[email protected]>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces significant modifications to the virtual contributor creation workflow across multiple files. The changes focus on enhancing the process of creating virtual contributors by updating GraphQL mutations, refactoring form handling, and streamlining the creation logic. The modifications include adding a knowledge base field to the mutation, improving form submission handling, and simplifying the virtual contributor wizard's internal logic. Changes
Sequence DiagramsequenceDiagram
participant User
participant VCWizard as Virtual Contributor Wizard
participant Form as Content Addition Form
participant Backend as Backend Service
User->>VCWizard: Initiate VC Creation
VCWizard->>Form: Open Content Addition Form
User->>Form: Fill Content Details
Form->>VCWizard: Submit Form Data
VCWizard->>Backend: Create Virtual Contributor
Backend-->>VCWizard: Confirmation
VCWizard->>User: Display Success Message
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
…misleading createdSpaceId usage;
bobbykolev
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx (3)
Line range hint
38-51: Clarify step usage to reflect new BoK concept.You’ve introduced the
'insufficientPrivileges'step but note in the comment that it’s currently unused. Consider whether to remove it if it’s truly not used or if you plan to leverage it soon (e.g., for permission checks in the future).
209-270: Excellent consolidation of VC creation logic.The
executeVcCreationfunction provides a solid approach for creating a new VC with descriptive data. Ensure that any unhandled exceptions in the upstream GraphQL API do not silently fail. You may want to add logging or a user alert in the catch block if it fails beyond basic error handling.Do you want assistance adding more robust error handling or validation around the server’s response?
272-327: Sequential awaits could slow performance.When adding the VC to the community or retrieving queries, any repeated sequential
awaitcalls might be slow. Consider parallelizing tasks withPromise.allif they’re independent.src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContentForm.tsx (1)
40-46: Load indicator clarity.By setting
isSubmittedto true/false aroundonSubmit, you provide clear user feedback. Ensure edge cases (e.g., network errors) reset the submission state properly if the request fails.src/domain/journey/space/pages/SpaceSettings/VirtualContributor.graphql (1)
9-24: The knowledgeBase structure looks well-organized.The nested structure follows GraphQL best practices and clearly represents the relationships between BoK, calloutsSet, and callouts.
Consider adding field descriptions using GraphQL descriptions to improve schema documentation:
+""" +Knowledge base (BoK) associated with the virtual contributor +""" knowledgeBase { id + """ + Set of callouts organized within this knowledge base + """ calloutsSet { id callouts { id framing { id profile { id displayName } } } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/core/apollo/generated/apollo-hooks.tsis excluded by!**/generated/**src/core/apollo/generated/graphql-schema.tsis excluded by!**/generated/**
📒 Files selected for processing (6)
src/domain/journey/space/pages/SpaceSettings/VirtualContributor.graphql(1 hunks)src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContent.tsx(1 hunks)src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContentForm.tsx(4 hunks)src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContentProps.ts(2 hunks)src/main/topLevelPages/myDashboard/newVirtualContributorWizard/CreateNewVirtualContributor.tsx(3 hunks)src/main/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx(11 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/CreateNewVirtualContributor.tsx (1)
Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Use the following code guide documentation in the codebase: 'docs/code-guidelines.md'.
Ensure sufficient error handling and logging is present, but note:
- We have a global error handler in place. So local
try/catchblocks are not required unless they provide value beyond what the global error handler already covers. - Avoid flagging the absence of local error handling in cases where the global error handler suffices.
- Avoid flagging the absence of logging in cases where the global logger is already in place (graphql usage).
Inform about the recommendation of not using non-null assertion,
except on GraphQL queries where the variable is asserted non-null and followed by a
skip: with the same variable.
Examples:
- Not Required: there's a skip
{
variables: { templateId: templateId! },
skip: !templateId,
}
Check for common security vulnerabilities such as:
- SQL Injection
- XSS (Cross-Site Scripting)
- CSRF (Cross-Site Request Forgery)
- Insecure dependencies
- Sensitive data exposure
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContent.tsx (1)
Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Use the following code guide documentation in the codebase: 'docs/code-guidelines.md'.
Ensure sufficient error handling and logging is present, but note:
- We have a global error handler in place. So local
try/catchblocks are not required unless they provide value beyond what the global error handler already covers. - Avoid flagging the absence of local error handling in cases where the global error handler suffices.
- Avoid flagging the absence of logging in cases where the global logger is already in place (graphql usage).
Inform about the recommendation of not using non-null assertion,
except on GraphQL queries where the variable is asserted non-null and followed by a
skip: with the same variable.
Examples:
- Not Required: there's a skip
{
variables: { templateId: templateId! },
skip: !templateId,
}
Check for common security vulnerabilities such as:
- SQL Injection
- XSS (Cross-Site Scripting)
- CSRF (Cross-Site Request Forgery)
- Insecure dependencies
- Sensitive data exposure
src/domain/journey/space/pages/SpaceSettings/VirtualContributor.graphql (1)
Pattern src/**/*.{graphql,gql}: Review the GraphQL schema and queries for best practices, potential bugs, and adherence to the project's GraphQL standards.
Ensure that the schema is well-defined and queries are optimized.
Check for security vulnerabilities.
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContentForm.tsx (1)
Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Use the following code guide documentation in the codebase: 'docs/code-guidelines.md'.
Ensure sufficient error handling and logging is present, but note:
- We have a global error handler in place. So local
try/catchblocks are not required unless they provide value beyond what the global error handler already covers. - Avoid flagging the absence of local error handling in cases where the global error handler suffices.
- Avoid flagging the absence of logging in cases where the global logger is already in place (graphql usage).
Inform about the recommendation of not using non-null assertion,
except on GraphQL queries where the variable is asserted non-null and followed by a
skip: with the same variable.
Examples:
- Not Required: there's a skip
{
variables: { templateId: templateId! },
skip: !templateId,
}
Check for common security vulnerabilities such as:
- SQL Injection
- XSS (Cross-Site Scripting)
- CSRF (Cross-Site Request Forgery)
- Insecure dependencies
- Sensitive data exposure
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContentProps.ts (1)
Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Use the following code guide documentation in the codebase: 'docs/code-guidelines.md'.
Ensure sufficient error handling and logging is present, but note:
- We have a global error handler in place. So local
try/catchblocks are not required unless they provide value beyond what the global error handler already covers. - Avoid flagging the absence of local error handling in cases where the global error handler suffices.
- Avoid flagging the absence of logging in cases where the global logger is already in place (graphql usage).
Inform about the recommendation of not using non-null assertion,
except on GraphQL queries where the variable is asserted non-null and followed by a
skip: with the same variable.
Examples:
- Not Required: there's a skip
{
variables: { templateId: templateId! },
skip: !templateId,
}
Check for common security vulnerabilities such as:
- SQL Injection
- XSS (Cross-Site Scripting)
- CSRF (Cross-Site Request Forgery)
- Insecure dependencies
- Sensitive data exposure
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx (1)
Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Use the following code guide documentation in the codebase: 'docs/code-guidelines.md'.
Ensure sufficient error handling and logging is present, but note:
- We have a global error handler in place. So local
try/catchblocks are not required unless they provide value beyond what the global error handler already covers. - Avoid flagging the absence of local error handling in cases where the global error handler suffices.
- Avoid flagging the absence of logging in cases where the global logger is already in place (graphql usage).
Inform about the recommendation of not using non-null assertion,
except on GraphQL queries where the variable is asserted non-null and followed by a
skip: with the same variable.
Examples:
- Not Required: there's a skip
{
variables: { templateId: templateId! },
skip: !templateId,
}
Check for common security vulnerabilities such as:
- SQL Injection
- XSS (Cross-Site Scripting)
- CSRF (Cross-Site Request Forgery)
- Insecure dependencies
- Sensitive data exposure
🔇 Additional comments (14)
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx (4)
1-23: Use descriptive import management and ensure consistent usage.The newly introduced and updated imports look reasonable. However, double-check that each imported hook or type is being consistently utilized and that there are no stale references to subspace-specific code.
Line range hint
89-129: Potential confusion in auto-selecting the first space.At lines 90-101, you automatically pick the first space from
mySpaces. This could lead to confusion or unintended usage if the user has multiple spaces. Consider prompting the user or providing a UI-driven choice.Would you like help refactoring to let the user explicitly select the space?
424-442: Successful integration of existing knowledge references.The
handleCreateVCWithExistingKnowledgefunction nicely ties the existing knowledge’sidandparentRoleSetIdto the VC creation. Just ensure that the user is aware of limitations (e.g., the user might not have privileges on that existing space).
Line range hint
446-460: External provider support looks well-structured.Using
externalConfigfor storing API keys and assistant IDs is clean. Confirm that sensitive data is stored securely on the backend. Watch out for any potential exposure in logs or publicly visible fields.src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContentProps.ts (3)
1-2: Appropriate usage of GraphQL enumeration.Importing
CalloutState,CalloutType, andCalloutVisibilityfrom the GraphQL schema is aligned with typed usage. No issues found here.
23-38: Post callout data is well-structured.The
getPostCalloutRequestDatafunction is concise and follows a consistent shape for the callout payload. This direct approach is simpler than constructing nested objects inline. Good job.
40-54: Enable extension for documents callout.
getDocumentCalloutRequestDataparallels the post callout structure, but with an openCalloutState. Ensure any future advanced properties also remain consistent.src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContent.tsx (1)
22-22: Conditional context usage for storage.Setting
locationType={spaceId ? 'journey' : 'platform'}is a noteworthy improvement. This ensures that, when nospaceIdis present, you default to a more generic storage context. Keep an eye out for possible confusion if the user’s environment expects a journey-based location.src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContentForm.tsx (2)
37-37: Async submission design is appropriate.Switching
onSubmitto aPromise<void>is a good change to accommodate asynchronous operations cleanly.
203-205: Direct form submission.Line 205 ensures you invoke
handleSubmit({ posts, documents })while showing a loading indicator. This is user-friendly. Expand instructions or tooltips if additional user steps are needed after success.src/main/topLevelPages/myDashboard/newVirtualContributorWizard/CreateNewVirtualContributor.tsx (3)
27-27: Parameter name aligns better with context.Renaming
onCreateSpacetoonCreateKnowledgehelps clarify what the function does based on the new feature. Good naming choice.
83-83: Clean injection of props.Switching to
onCreateKnowledgein the component’s props is consistent with the type changes. The naming is now more explicit regarding your new BoK concept.
118-118: Logic for new BoK source.
case VCSourceOptions.WRITTEN_KNOWLEDGE:is a correct approach to seamlessly handle the new creation flow for BoK-based VCs. Just confirm that future expansions (e.g., distinguishing multiple knowledge base types) remain backward-compatible.src/domain/journey/space/pages/SpaceSettings/VirtualContributor.graphql (1)
Line range hint
1-24: Verify authorization checks for the createVirtualContributor mutation.The PR objectives mention commented-out privilege checks. Ensure proper authorization is implemented for this mutation.
Let's check for any authorization directives or types in the schema:
...ain/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx
Show resolved
Hide resolved
...ain/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx
Show resolved
Hide resolved
...ain/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx
Outdated
Show resolved
Hide resolved
This branch will be the feature branch of alkem-io/alkemio#1467
In order to be shippable we need:
Details:
Issues:
#7379
#6764
Current vs new flow WB:
https://alkem.io/building-alkemio-org/challenges/technicaldesigns-9853/collaboration/knowledgebasevcs
What's done
The Written Knowledge step now:
AlkemioKnowledgeBaseand instead in Subspace, the callouts are under the VC entity;Known issues:
CLIENT
SERVER - alkem-io/server#4812
Summary by CodeRabbit
New Features
Bug Fixes
Refactor