Links & Docs to BoK on VC creation #7365
Conversation
WalkthroughThis pull request introduces enhancements to the Virtual Contributor creation process by adding support for document management. Changes include updating the translation file with a new Changes
Suggested labels
Suggested reviewers
Possibly related PRs
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
|
bobbykolev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/DocumentItem.tsx (1)
18-62: Good approach for handling document upload references.The usage of FormikFileInput plus contextual helper text is user-friendly. Just ensure that the final upload or URL references are validated where it matters (e.g., server side or a global handler) to prevent any security vulnerabilities.
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContentForm.tsx (2)
102-109: Avoid redundant Boolean conversion.At line 169, the code uses “Boolean(posts.length)”. Since “posts.length” is already coerced to a boolean in logical expressions, removing the explicit “Boolean()” wrapper is more concise and avoids unnecessary overhead.
- title={ - Boolean(posts.length) - ? undefined - : t('createVirtualContributorWizard.addContent.submitDisabled') - } + title={ + posts.length + ? undefined + : t('createVirtualContributorWizard.addContent.submitDisabled') + }
129-136: Remove the unnecessary React fragment.At line 134, the empty fragment “<></>” doesn’t wrap multiple elements, so it can be removed without affecting the layout.
- tooltip: ( - <Tooltip - title={t('createVirtualContributorWizard.addContent.documents.tooltip')} - arrow - placement="top" - > - <></> - </Tooltip> - ), + tooltip: ( + <Tooltip + title={t('createVirtualContributorWizard.addContent.documents.tooltip')} + arrow + placement="top" + /> + ),🧰 Tools
🪛 Biome (1.9.4)
[error] 134-134: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/core/i18n/en/translation.en.json(1 hunks)src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent.tsx(0 hunks)src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContent.tsx(1 hunks)src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContentForm.tsx(1 hunks)src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContentProps.ts(1 hunks)src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/DocumentItem.tsx(1 hunks)src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/PostItem.tsx(1 hunks)src/main/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx(8 hunks)
💤 Files with no reviewable changes (1)
- src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent.tsx
🧰 Additional context used
📓 Path-based instructions (7)
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/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/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/PostItem.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/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
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/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/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/core/i18n/en/translation.en.json (1)
Pattern src/**/*.json: Review the JSON files for correct syntax and structure.
Ensure that the configuration and data are accurate and follow the project's standards.
Check for sensitive data exposure and ensure that the data is properly validated.
🪛 Biome (1.9.4)
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContentForm.tsx
[error] 169-169: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
[error] 134-134: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx
[error] 456-456: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
[error] 470-470: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
🔇 Additional comments (12)
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContentProps.ts (4)
1-4: Type definitions for PostValues look good.
The field names are clear, and the types make sense for basic text-based posts.
11-14: The BoKCalloutsFormValues interface is well-structured for holding multiple items.
No issues spotted. Ensure any form hooks or schema validations handle these arrays properly.
16-20: AddContentProps is concise and consistent.
Props appear to match the usage context: onClose and onCreateVC are well-defined, and spaceId is typed as a string. Good to go.
6-9: Consider validating the URL in DocumentValues if necessary.
While the interface is straightforward, if malicious or unexpected URLs are possible, you may need stronger validation in the consuming logic or server-side verification.
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/AddContent.tsx (4)
13-15: Hook usage for translation and state management is correct.
Well-implemented with modern React hooks. Good structure for maintainability.
17-19: onCancel logic is straightforward.
Toggling dialog state for cancellation is appropriate. No suggestions needed here.
21-33: Clean layout and integration with Dialog components.
The combination of DialogHeader, DialogContent, and CancelDialog is clear and user-friendly. The StorageConfigContextProvider usage also cleanly provides context for file storage.
36-36: Default export consistent with project style.
No issues found.
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/PostItem.tsx (2)
12-17: Prop definitions for PostItemProps are coherent.
The structure fits well with typical list item usage, allowing for index-based deletions.
19-54: Ensure that markdown content is sanitized if rendered as HTML.
When displaying user-entered content, confirm that any rendering logic (outside this component) safely sanitizes to prevent potential XSS.
✅ Verification successful
Let me search for any markdown rendering logic in the codebase to verify the security concerns.
Let me check the implementation of the markdown rendering component to verify if proper sanitization is in place.
Let me check the UnifiedConverter implementation to verify the sanitization process.
Proper sanitization is confirmed through rehype-sanitize implementation
The codebase properly implements content sanitization through the rehype-sanitize package in the UnifiedConverter component. Specifically:
- The markdown-to-HTML conversion pipeline uses
rehype-sanitizewith a custom sanitization schema - The sanitization is applied before the final HTML rendering
- The implementation extends the default schema with carefully controlled allowances for iframes
- The package
rehype-sanitizeis properly listed in the project dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for markdown rendering components or utilities
rg -l "markdown|sanitize|xss|dangerouslySetInnerHTML" --type tsx --type ts
# Search for FormikMarkdownField implementation
ast-grep --pattern 'const FormikMarkdownField = $_'
# Look for markdown-related dependencies in package.json
cat package.json | jq '.dependencies,.devDependencies' | grep -i "markdown\|sanitize"
Length of output: 11233
Script:
#!/bin/bash
# Search for MarkdownInput implementation and usage of react-markdown
rg -A 5 "react-markdown|rehype-sanitize" --type tsx --type ts
# Look for the MarkdownInput component implementation
ast-grep --pattern 'const MarkdownInput = $_'
Length of output: 21713
Script:
#!/bin/bash
# Search for UnifiedConverter implementation
ast-grep --pattern 'const UnifiedConverter = $_'
# Look for any sanitization related imports or configurations
rg -A 5 "rehype-sanitize|sanitize" --type ts --type tsx --type js --type jsx
Length of output: 10248
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/AddContent/DocumentItem.tsx (1)
12-16: DocumentItemProps interface matches typical document handling needs.
The index-based approach for deletion will be easy to integrate into an array of documents.
src/core/i18n/en/translation.en.json (1)
3148-3156: Translations look good.
The newly added keys for “documents” provide clarity for the user interface and align well with the existing structure. No syntax anomalies found.
...ain/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx
Outdated
Show resolved
Hide resolved
...ain/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx
Outdated
Show resolved
Hide resolved
...ain/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx
Outdated
Show resolved
Hide resolved
...ain/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx (3)
449-461: Sequential post creation works but could be parallelized.
This logic is correct and includes a fallback array to avoid null reference errors. For a large set of posts, consider usingPromise.allto create them in parallel and improve performance.
463-475: Documents creation approach mirrors posts, but parallel creation may help.
Creating each document link in a loop is functional. If the volume of documents can be large, consider batching or parallel creation withPromise.allfor efficiency.
664-668: Prop naming consistency recommendation.
While the flow is correct, renamingonCreateVCto something likeonAddContentmay better reflect its call tohandleAddContent. This is a minor readability improvement.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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 (7)
src/main/topLevelPages/myDashboard/newVirtualContributorWizard/useNewVirtualContributorWizard.tsx (7)
Line range hint 1-16: Imports look consistent and correct.
These imports, including the new useCreateLinkOnCalloutMutation, are well-structured and conform to the existing Apollo and React patterns. No issues found.
32-33: New AddContent imports are valid.
Bringing in AddContent and the related type definitions (BoKCalloutsFormValues, DocumentValues, PostValues) is aligned with the new workflow for handling both posts and documents.
93-95: Refactored state aligns with new multi-content design.
Renaming calloutPostData to calloutData and using a BoKCalloutsFormValues type is consistent with handling both posts and documents under one interface.
108-109: Function updated to handle new callout data.
handleAddContent sets calloutData in state. The logic straightforwardly triggers the downstream creation process once the state is updated.
392-408: Callout settings for document links look good.
Defining a separate calloutDocumentsDetails object for LinkCollection callouts is a clear approach for managing document references. This mirrors the approach used for posts.
428-444: Link creation mutation is properly set up.
onCreateLink leverages createLinkOnCallout to store document data. This follows the existing pattern for posts and ensures consistent handling of the newly introduced documents.
478-479: Flag reset is straightforward.
Toggling setTryCreateCallout(false) wraps up the creation phase. No issues found here.
* 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]>
Now the first option in VC creation allows uploading of documents.
The docs are temporarily stored under the BoK's Space Storage.
In addition, a small refactoring was made organizing better the 'AddContent' step of the VC creation flow.
Summary by CodeRabbit
New Features
Bug Fixes