-
Notifications
You must be signed in to change notification settings - Fork 6
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
Links and Docs - forgotten commit with Validation #7377
Conversation
# Conflicts: # src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContentForm.tsx # src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/DocumentItem.tsx # src/main/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx
WalkthroughThis pull request enhances the Virtual Contributor wizard's document management functionality by introducing new features for adding and validating documents. The changes span across three files: Changes
Sequence DiagramsequenceDiagram
participant User
participant AddContentForm
participant DocumentItem
participant VCWizard
User->>AddContentForm: Enters document details
AddContentForm->>DocumentItem: Passes document data
DocumentItem->>AddContentForm: Updates document information
AddContentForm->>VCWizard: Submits validated documents
VCWizard->>VCWizard: Create posts and links
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
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/DocumentItem.tsx (2)
1-1
: Consider avoidinglodash/noop
if we only need a trivial no-operation.
Thenoop
import from lodash works fine, but if your codebase is trying to reduce dependencies, a locally defined empty function or optional chaining could suffice. If lodash is already used extensively, this is inconsequential.Also applies to: 11-11
17-17
: Review the combined function type foronChange
.
You’ve combinedChangeEventHandler<HTMLInputElement | HTMLTextAreaElement>
with a callback accepting astring
. This may be suitable, but it can also become confusing if the signature grows. Consider splitting them into separate, clearly named handlers if this gets more complex.src/main/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx (1)
457-460
: Sequential creation of posts inside a for-loop.
This logic is straightforward but creates posts one by one, awaiting each. If many posts are expected, consider parallel operations (e.g.,Promise.all()
) if that’s permissible for the backend. Currently, an error on one post stops the loop, which may be acceptable or undesirable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContentForm.tsx
(4 hunks)src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/DocumentItem.tsx
(4 hunks)src/main/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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/catch
blocks 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/DocumentItem.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/catch
blocks 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/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/catch
blocks 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 (6)
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/DocumentItem.tsx (2)
20-20
: Providing a default noop
avoids undefined checks.
This approach ensures no runtime errors if onChange
is omitted. The usage is straightforward and effectively eliminates extra conditionals.
34-34
: Validation alignment between UI and form schema.
The newly added required
props on both fields align well with the corresponding validations in the form schema. Passing onChange={onChange}
to FormikFileInput
is correct but ensure that FormikFileInput
invokes onChange
only once so it doesn’t conflict with Formik’s internal event handlers.
Also applies to: 40-40, 49-49
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContentForm.tsx (3)
54-66
: Good use of validation schema for documents
.
The constraints for name
and url
are clear. You might consider validating the URL format if needed (e.g., using url()
in yup). Otherwise, this is an effective inclusion of required fields and length checks.
85-85
: validateOnMount
ensures immediate validation but may cause early user feedback.
Enabling validateOnMount
will mark the form invalid until users interact. This may be intended or could confuse new users. Confirm this is desirable for the user experience.
154-160
: Dynamically updating documents
array with onChange
.
Mapping the file name directly to documents[index].name
is a neat approach. Confirm that empty or invalid file names are handled gracefully and that you only intend to capture the file’s basename or a user-friendly reference if needed.
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx (1)
473-476
: Document creation is also done sequentially.
As with the posts, you may parallelize the creation of documents if your backend can handle it. Otherwise, sequential creation ensures an error halts creation, which might be a deliberate design.
…#7381) * added new tabs to user + org admin pages; refactored how admin pages for users + orgs + vcs are managed; moved some global admin functionality out of domain down to platform admin; ... * updated generation to match api tidy ups related to set of preference types + ID passing for org mutations * fix compile errors related to dropping of separate UserPreferenceType enum * Synchronize icons, remove comments, make sure there are no redundant settings calls. * Links & Docs to BoK on VC creation (#7365) * 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]> * Links and Docs - forgotten commit with Validation (#7377) * CalloutsSet entity (#7376) * 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]> * VC knowledge base instead of subspace init * Space creation after VC creation, loading, code opt & reorganization * Fix docs uploading, code organization and documentation; * fix uploading of docs in case there's no space under the acc; remove misleading createdSpaceId usage; * useLoadingState instead of a new React State * Fix - set properly the persona type depending on the 3 steps; * Ability to select SpaceLevel2 on create VC (#7386) * VC Knowledge Base callouts dialog (#7388) * VC Knowledge Base callouts dialog - init. * Filter available callout types. * disable rich media on VC callout creation. * Description component with update functionality. * Update the Create Written Knowledge UI and initial state; Fix dialog titles in VC flow. * Reingest logic in the Knowledge dialog. * Remove the icon logic for CalloutVisibilityChangeDialog. * Use the account hostname for space created in the VC flow. * fix VC dialog not opening; remove outdated copy; * storage config for KnowledgeBase description --------- Co-authored-by: Neil Smyth <[email protected]> Co-authored-by: Petar Kolev <[email protected]> Co-authored-by: reactoholic <[email protected]> Co-authored-by: Neil Smyth <[email protected]> Co-authored-by: Carlos Cano <[email protected]>
missing commit in #7365
Summary by CodeRabbit
New Features
Bug Fixes
Improvements