fix: added workspaceslug in renderChildren of project settings #5951
Conversation
|
Warning Rate limit exceeded@gakshita has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request involve modifications to the Changes
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/ce/constants/project/settings/features.tsx (1)
16-17: Consider making workspaceSlug parameter optionalSince this is a breaking change, consider making the
workspaceSlugparameter optional (workspaceSlug?: string) if it's not strictly required for all implementations. This would make the change backward compatible.renderChildren?: ( currentProjectDetails: IProject, isAdmin: boolean, handleSubmit: (featureKey: string, featureProperty: string) => Promise<void>, - workspaceSlug: string + workspaceSlug?: string ) => ReactNode;web/core/components/project/settings/features-list.tsx (1)
105-105: LGTM! The workspaceSlug parameter is correctly passed.The addition of workspaceSlug parameter to renderChildren aligns with the PR objectives and type definition changes.
Consider adding a null check for better safety:
-featureItem.renderChildren(currentProjectDetails, isAdmin, handleSubmit, workspaceSlug)} +featureItem.renderChildren(currentProjectDetails, isAdmin, handleSubmit, workspaceSlug ?? '')}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/ce/constants/project/settings/features.tsx(1 hunks)web/core/components/project/settings/features-list.tsx(1 hunks)
🔇 Additional comments (1)
web/ce/constants/project/settings/features.tsx (1)
16-17: Breaking change: Verify all implementations of renderChildren
The addition of the workspaceSlug parameter to the renderChildren function signature is a breaking change. While this change aligns with the PR objectives, we need to ensure all implementations of this type are updated accordingly.
Let's verify all implementations of renderChildren:
✅ Verification successful
Let me gather more information to verify the usage and implementation of renderChildren.
Based on the search results, I can now provide a final response as we have sufficient information about the implementation and usage:
Breaking change verification completed - Implementation is consistent
The workspaceSlug parameter addition to renderChildren is properly handled:
- The type definition in
web/ce/constants/project/settings/features.tsxmatches its usage inweb/core/components/project/settings/features-list.tsx - The implementation correctly passes the
workspaceSlugparameter:featureItem.renderChildren(currentProjectDetails, isAdmin, handleSubmit, workspaceSlug) - All components using
ProjectFeaturesListprovide the requiredworkspaceSlugparameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any implementations of renderChildren to ensure they're updated
# with the new workspaceSlug parameter
# Look for component implementations that might use renderChildren
rg -l "renderChildren.*=.*\("
# Look for direct usage of TProperties type
rg -l "TProperties"
Length of output: 98
Script:
#!/bin/bash
# Search for the actual implementation and usage of renderChildren with context
rg "renderChildren" -A 5 -B 5
# Also search for any ProjectFeaturesList component usage
rg "ProjectFeaturesList" -A 3 -B 3
Length of output: 5945
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
space/core/components/editor/rich-text-editor.tsx (1)
1-11: Clean up imports and add type documentation.Remove empty comments and add JSDoc documentation for better code maintainability.
import React, { forwardRef } from "react"; -// editor import { EditorRefApi, IMentionHighlight, IRichTextEditor, RichTextEditorWithRef } from "@plane/editor"; -// types -// helpers import { cn } from "@/helpers/common.helper"; import { getEditorFileHandlers } from "@/helpers/editor.helper"; +/** + * Props for the RichTextEditor component. + * Extends the base IRichTextEditor interface but handles fileHandler and mentionHandler internally. + */ interface RichTextEditorWrapperProps extends Omit<IRichTextEditor, "fileHandler" | "mentionHandler"> { uploadFile: (file: File) => Promise<string>; }web/core/services/inbox/inbox-issue.service.ts (1)
Line range hint
1-103: Consider splitting inbox and intake settings into separate services.The service currently handles both inbox issues and intake settings. Given the API restructuring (evidenced by the move from
/publish-intake/to/intake-settings/), consider splitting this into two services:
InboxIssueService: For issue-related operationsInboxSettingsService: For intake/form settings operationsThis would:
- Improve separation of concerns
- Make the codebase more maintainable
- Better reflect the API structure
web/core/store/inbox/project-inbox.store.ts (1)
332-347: LGTM! Robust implementation with optimistic updates and error handling.The implementation follows best practices:
- Stores initial state for potential rollback
- Updates local state optimistically
- Makes the API call
- Updates the anchor property from the response
- Rolls back on error
Consider adding a type guard for the API response to ensure type safety:
- const result = await this.inboxIssueService.updatePublishForm(workspaceSlug, projectId, data); + const result = await this.inboxIssueService.updatePublishForm(workspaceSlug, projectId, data); + if (!result?.anchor) return; runInAction(() => { set(this.intakeForms, projectId, { ...this.intakeForms[projectId], anchor: result?.anchor }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/types/src/inbox.d.ts(1 hunks)space/core/components/editor/rich-text-editor.tsx(1 hunks)space/core/store/issue-detail.store.ts(3 hunks)space/core/types/intake.d.ts(1 hunks)web/core/services/inbox/inbox-issue.service.ts(1 hunks)web/core/store/inbox/project-inbox.store.ts(2 hunks)
🔇 Additional comments (8)
space/core/types/intake.d.ts (1)
1-6: Consider enhancing type safety and flexibility.
A few suggestions to improve the type definition:
- Add email validation using a more specific type
- Consider making some fields optional
- Add sanitization note for HTML content
+// Regular expression for basic email validation
+type EmailString = string & { __emailBrand: never };
+
export type TIntakeIssueForm = {
name: string;
- email: string;
+ email: EmailString;
username: string;
+ // NOTE: Ensure this content is sanitized before rendering
description_html: string;
+ // Optional fields for flexibility
+ title?: string;
+ category?: string;
};Let's check if there's any HTML sanitization in place:
✅ Verification successful
HTML content is properly sanitized, but type improvements are still recommended
The codebase has robust HTML sanitization in place using isomorphic-dompurify and dompurify libraries:
sanitizeHTMLfunction in multiple helper files that sanitizes HTML content- Consistent usage of
DOMPurify.sanitize()with configurable allowed tags
Still recommend the type improvements for better type safety and flexibility:
+// Regular expression for basic email validation
+type EmailString = string & { __emailBrand: never };
+
export type TIntakeIssueForm = {
name: string;
- email: string;
+ email: EmailString;
username: string;
description_html: string;
+ title?: string;
+ category?: string;
};🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for HTML sanitization libraries or utilities
rg -i "sanitize|purify|dompurify|xss" --type ts
Length of output: 3016
space/core/components/editor/rich-text-editor.tsx (1)
41-41: LGTM!
Setting the display name is a good practice for debugging purposes.
packages/types/src/inbox.d.ts (1)
103-104: Verify the impact of splitting is_disabled into two separate flags.
The change from a single is_disabled flag to separate is_in_app_enabled and is_form_enabled flags provides more granular control over form functionality. This is a good improvement that allows for more flexible form state management.
Let's verify that all usages of the previous is_disabled property have been properly migrated:
✅ Verification successful
Let me gather more information to verify if there are any remaining references to is_disabled in the codebase that might need migration.
Let me check the implementation in the store and service files to understand how these properties are being used.
Based on the comprehensive analysis of the codebase, I can now provide the final response:
The change from is_disabled to separate enable flags is safe and properly implemented.
The code review shows that:
- There are no remaining references to
is_disabledin the codebase - The new flags (
is_in_app_enabledandis_form_enabled) are properly integrated into:- The type definition (
TInboxForm) - The intake form management in
project-inbox.store.ts - The API service layer in
inbox-issue.service.ts
- The type definition (
The implementation handles these flags consistently through the form management lifecycle (retrieve, update, regenerate), and there are no legacy references that need migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to is_disabled in TInboxForm context
# and verify the implementation of the new flags
# Search for any remaining references to is_disabled
echo "Checking for remaining references to is_disabled..."
rg "is_disabled.*TInboxForm"
# Search for implementations of the new flags to ensure consistent usage
echo "Checking implementations of new flags..."
rg -A 2 "is_(in_app|form)_enabled"
Length of output: 453
Script:
#!/bin/bash
# Search for any references to is_disabled in the entire codebase
echo "Checking for all references to is_disabled..."
rg "is_disabled"
# Search for files that might be using TInboxForm type
echo "Checking for files using TInboxForm..."
rg "TInboxForm"
Length of output: 1262
Script:
#!/bin/bash
# Check the implementation in project-inbox.store.ts
echo "Checking project-inbox store implementation..."
cat "web/core/store/inbox/project-inbox.store.ts"
# Check the implementation in inbox-issue.service.ts
echo "Checking inbox issue service implementation..."
cat "web/core/services/inbox/inbox-issue.service.ts"
Length of output: 24917
web/core/services/inbox/inbox-issue.service.ts (2)
87-88: Enhanced form control with granular settings.
The changes improve the API by:
- Accepting
Partial<TInboxForm>instead of a boolean, allowing more flexible form updates - Returning
Promise<TInboxForm>which better represents the response data
#!/bin/bash
# Description: Verify that all consumers of updatePublishForm have been updated
# Test: Search for method calls to ensure they're passing the correct type
ast-grep --pattern 'updatePublishForm($$$)'
# Test: Check for any remaining boolean-based calls that might have been missed
rg -A 3 "updatePublishForm.*true|updatePublishForm.*false" --type ts --type tsx80-80: Verify the API endpoint change consistency across the codebase.
The endpoint has been changed from /publish-intake/ to /intake-settings/. Let's ensure this change is consistent across the codebase.
space/core/store/issue-detail.store.ts (2)
39-39: LGTM: Interface addition follows existing patterns.
The new uploadIssueAsset method signature aligns well with the existing uploadCommentAsset method, maintaining consistency in the codebase.
83-83: LGTM: Proper MobX action registration.
The method is correctly registered as a MobX action, maintaining consistency with other file upload methods in the store.
web/core/store/inbox/project-inbox.store.ts (1)
74-74: Verify all callers of toggleIntakeForms are updated.
The method signature has changed from accepting a boolean isDisabled to accepting a Partial<TInboxForm>. This is a breaking change that requires updates in all calling code.
✅ Verification successful
Method signature change appears to be safe
The method toggleIntakeForms is only defined in the store interface and implementation, with no direct usage found in the codebase. Since it's a new change (as indicated by the review context) and no existing calls were found, this suggests the method is either:
- Being introduced for the first time, or
- Being updated alongside all its usages in the same PR
The lack of existing usage makes this change safe from a breaking change perspective.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to toggleIntakeForms to verify they've been updated
# Expected: All calls should pass a Partial<TInboxForm> object, not a boolean
# Search for method calls
rg -A 2 "toggleIntakeForms\("
Length of output: 31
Script:
#!/bin/bash
# Let's try a broader search pattern to catch any references to toggleIntakeForms
rg -i "toggleIntakeForms" --type ts --type tsx -A 3
# Also search for any form toggle related UI components that might be calling this
rg -i "form.*toggle|toggle.*form" --type ts --type tsx -A 3
Length of output: 169
Script:
#!/bin/bash
# Search for toggleIntakeForms without file type restrictions
rg -i "toggleIntakeForms" -A 3
# Also search for the store instance usage
rg -i "projectInboxStore" -A 3
Length of output: 3443
Script:
#!/bin/bash
# Let's find actual usage of the toggleIntakeForms method in components
rg -l "useProjectInbox" | xargs rg "toggleIntakeForms" -B 2 -A 2
# Also search for any form-related components that might be using this method
fd -e tsx -e ts | xargs rg "inbox.*form|form.*inbox" -B 2 -A 2
Length of output: 580
| mentionHandler={{ | ||
| highlights: function (): Promise<IMentionHighlight[]> { | ||
| throw new Error("Function not implemented."); | ||
| }, | ||
| suggestions: undefined, | ||
| }} |
There was a problem hiding this comment.
Implement mention handling instead of throwing an error.
The current implementation throws an error which could cause runtime issues. Either implement the mention handling functionality or return an empty array as a safer default.
mentionHandler={{
highlights: function (): Promise<IMentionHighlight[]> {
- throw new Error("Function not implemented.");
+ return Promise.resolve([]); // Safe default until implementation is ready
},
suggestions: undefined,
}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mentionHandler={{ | |
| highlights: function (): Promise<IMentionHighlight[]> { | |
| throw new Error("Function not implemented."); | |
| }, | |
| suggestions: undefined, | |
| }} | |
| mentionHandler={{ | |
| highlights: function (): Promise<IMentionHighlight[]> { | |
| return Promise.resolve([]); // Safe default until implementation is ready | |
| }, | |
| suggestions: undefined, | |
| }} |
| export const RichTextEditor = forwardRef<EditorRefApi, RichTextEditorWrapperProps>((props, ref) => { | ||
| const { containerClassName, uploadFile, ...rest } = props; | ||
| // store hooks | ||
|
|
||
| // use-mention | ||
|
|
||
| // file size | ||
|
|
||
| return ( | ||
| <RichTextEditorWithRef | ||
| mentionHandler={{ | ||
| highlights: function (): Promise<IMentionHighlight[]> { | ||
| throw new Error("Function not implemented."); | ||
| }, | ||
| suggestions: undefined, | ||
| }} | ||
| ref={ref} | ||
| fileHandler={getEditorFileHandlers({ | ||
| uploadFile, | ||
| workspaceId: "", | ||
| anchor: "", | ||
| })} | ||
| {...rest} | ||
| containerClassName={cn("relative pl-3 pb-3", containerClassName)} | ||
| /> | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file uploads.
The uploadFile function is used directly without error handling. Consider wrapping it in a try-catch block and showing appropriate error messages to users.
export const RichTextEditor = forwardRef<EditorRefApi, RichTextEditorWrapperProps>((props, ref) => {
const { containerClassName, uploadFile, ...rest } = props;
- // store hooks
-
- // use-mention
-
- // file size
+ const handleFileUpload = async (file: File): Promise<string> => {
+ try {
+ return await uploadFile(file);
+ } catch (error) {
+ console.error('File upload failed:', error);
+ throw new Error('Failed to upload file. Please try again.');
+ }
+ };
return (
<RichTextEditorWithRef
mentionHandler={{...}}
ref={ref}
fileHandler={getEditorFileHandlers({
- uploadFile,
+ uploadFile: handleFileUpload,
workspaceId: "",
anchor: "",
})}
{...rest}
containerClassName={cn("relative pl-3 pb-3", containerClassName)}
/>
);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const RichTextEditor = forwardRef<EditorRefApi, RichTextEditorWrapperProps>((props, ref) => { | |
| const { containerClassName, uploadFile, ...rest } = props; | |
| // store hooks | |
| // use-mention | |
| // file size | |
| return ( | |
| <RichTextEditorWithRef | |
| mentionHandler={{ | |
| highlights: function (): Promise<IMentionHighlight[]> { | |
| throw new Error("Function not implemented."); | |
| }, | |
| suggestions: undefined, | |
| }} | |
| ref={ref} | |
| fileHandler={getEditorFileHandlers({ | |
| uploadFile, | |
| workspaceId: "", | |
| anchor: "", | |
| })} | |
| {...rest} | |
| containerClassName={cn("relative pl-3 pb-3", containerClassName)} | |
| /> | |
| ); | |
| }); | |
| export const RichTextEditor = forwardRef<EditorRefApi, RichTextEditorWrapperProps>((props, ref) => { | |
| const { containerClassName, uploadFile, ...rest } = props; | |
| const handleFileUpload = async (file: File): Promise<string> => { | |
| try { | |
| return await uploadFile(file); | |
| } catch (error) { | |
| console.error('File upload failed:', error); | |
| throw new Error('Failed to upload file. Please try again.'); | |
| } | |
| }; | |
| return ( | |
| <RichTextEditorWithRef | |
| mentionHandler={{ | |
| highlights: function (): Promise<IMentionHighlight[]> { | |
| throw new Error("Function not implemented."); | |
| }, | |
| suggestions: undefined, | |
| }} | |
| ref={ref} | |
| fileHandler={getEditorFileHandlers({ | |
| uploadFile: handleFileUpload, | |
| workspaceId: "", | |
| anchor: "", | |
| })} | |
| {...rest} | |
| containerClassName={cn("relative pl-3 pb-3", containerClassName)} | |
| /> | |
| ); | |
| }); |
| fileHandler={getEditorFileHandlers({ | ||
| uploadFile, | ||
| workspaceId: "", | ||
| anchor: "", | ||
| })} |
There was a problem hiding this comment.
💡 Codebase verification
Empty workspaceId and anchor parameters will cause file operations to fail
Based on the editor helper implementation, both workspaceId and anchor parameters are required for:
- Generating asset source URLs via
getEditorAssetSrc - Deleting assets via
fileService.deleteOldEditorAssetandfileService.deleteNewAsset - Restoring assets via
fileService.restoreOldEditorAssetandfileService.restoreNewAsset
The RichTextEditor component should:
- Accept
workspaceIdandanchoras props - Pass these values to
getEditorFileHandlersinstead of empty strings
Example locations requiring fixes:
space/core/components/editor/rich-text-editor.tsx: Add props and pass them to file handler
🔗 Analysis chain
Verify workspaceId and anchor parameters.
Empty strings are being passed for workspaceId and anchor. These should likely be provided through props or context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these parameters are used meaningfully in the editor file handlers
rg -A 5 "workspaceId|anchor" "$(fd -t f "editor.helper")"
Length of output: 217
Script:
#!/bin/bash
# Let's check the content of the editor helper file
cat space/helpers/editor.helper.ts
Length of output: 2257
Script:
#!/bin/bash
# Let's also check where getEditorFileHandlers is used and its implementation
ast-grep --pattern 'getEditorFileHandlers'
Length of output: 1417
Script:
#!/bin/bash
# Search for any file upload related functionality
rg -A 5 "uploadFile.*workspaceId|anchor.*upload"
Length of output: 774
Script:
#!/bin/bash
# Let's check the implementation of rich-text-editor.tsx to see how these parameters should be passed
cat space/core/components/editor/rich-text-editor.tsx
Length of output: 1232
Script:
#!/bin/bash
# Let's also check other implementations to see how they handle these parameters
rg -l "workspaceId.*anchor.*uploadFile" space/core/components/editor/
Length of output: 71
| uploadIssueAsset = async (file: File, anchor: string, commentID?: string) => { | ||
| try { | ||
| const res = await this.fileService.uploadAsset( | ||
| anchor, | ||
| { | ||
| entity_identifier: commentID ?? "", | ||
| entity_type: EFileAssetType.ISSUE_ATTACHMENT, | ||
| }, | ||
| file | ||
| ); | ||
| return res; | ||
| } catch (error) { | ||
| console.log("Error in uploading comment asset:", error); | ||
| throw new Error("Asset upload failed. Please try again later."); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and message accuracy.
Several improvements can be made to enhance reliability and maintainability:
- The error message incorrectly refers to "comment asset" instead of "issue asset"
- The
commentIDparameter usage could be more explicit - Error handling could be more specific
Consider applying these changes:
uploadIssueAsset = async (file: File, anchor: string, commentID?: string) => {
try {
const res = await this.fileService.uploadAsset(
anchor,
{
- entity_identifier: commentID ?? "",
+ entity_identifier: commentID || "",
entity_type: EFileAssetType.ISSUE_ATTACHMENT,
},
file
);
return res;
} catch (error) {
- console.log("Error in uploading comment asset:", error);
+ console.error("Error in uploading issue asset:", error);
+ if (error instanceof Error) {
+ throw error;
+ }
throw new Error("Asset upload failed. Please try again later.");
}
};Additionally, consider adding JSDoc documentation to clarify the purpose of the commentID parameter and when it should be provided.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uploadIssueAsset = async (file: File, anchor: string, commentID?: string) => { | |
| try { | |
| const res = await this.fileService.uploadAsset( | |
| anchor, | |
| { | |
| entity_identifier: commentID ?? "", | |
| entity_type: EFileAssetType.ISSUE_ATTACHMENT, | |
| }, | |
| file | |
| ); | |
| return res; | |
| } catch (error) { | |
| console.log("Error in uploading comment asset:", error); | |
| throw new Error("Asset upload failed. Please try again later."); | |
| } | |
| }; | |
| uploadIssueAsset = async (file: File, anchor: string, commentID?: string) => { | |
| try { | |
| const res = await this.fileService.uploadAsset( | |
| anchor, | |
| { | |
| entity_identifier: commentID || "", | |
| entity_type: EFileAssetType.ISSUE_ATTACHMENT, | |
| }, | |
| file | |
| ); | |
| return res; | |
| } catch (error) { | |
| console.error("Error in uploading issue asset:", error); | |
| if (error instanceof Error) { | |
| throw error; | |
| } | |
| throw new Error("Asset upload failed. Please try again later."); | |
| } | |
| }; |
Summary
This PR implements
Summary by CodeRabbit
Summary by CodeRabbit
New Features
renderChildrenmethod to accept an additionalworkspaceSlugparameter, improving context for child components in the feature list.RichTextEditorcomponent for improved text editing capabilities.Bug Fixes
Documentation