Release 9 patch into develop #7640
Conversation
Fix PROD issue with CallForPosts contributions failing to load
Release 9 to master
* loadFiles behaviour improvements * added comments and added one early exit to loadFiles * more code improvements
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThis pull request updates the package version and refactors several components. The changes include a minor version bump in package.json, refactoring of callback functions and memoization in Whiteboard-related components, enhanced state management and debounced event handling for the Excalidraw wrapper, adjustments in the file reconciliation process in the collaboration module, improved error handling in file downloads, and added entitlement checks with consolidated deletion logic in the contributor account view. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as Collab Module
participant F as File Loader
participant S as Remote Server
U->>C: Request whiteboard update
C->>F: Trigger file-loading (reconcileElementsAndLoadFiles)
F->>S: Download required files
S-->>F: Return file data
F-->>C: Deliver loaded files
C-->>U: Return reconciled elements with files loaded
sequenceDiagram
participant U as User
participant CA as ContributorAccountView
participant DM as Deletion Mutation
participant N as Notification System
U->>CA: Initiate deletion for an entity (e.g., Space)
CA->>CA: Call deleteEntity (switch-case based on entity type)
CA->>DM: Execute deletion mutation for selected entity type
DM-->>CA: Return success response
CA->>N: Display success notification
CA->>CA: Call clearDeleteState to reset deletion dialog
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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
|
# Conflicts: # package-lock.json # package.json # src/domain/collaboration/whiteboard/WhiteboardDialog/WhiteboardDialog.tsx # src/domain/common/whiteboard/excalidraw/CollaborativeExcalidrawWrapper.tsx # src/domain/community/contributor/Account/ContributorAccountView.tsx
|
@coderabbitai skip review |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/domain/community/contributor/Account/ContributorAccountView.tsx (1)
244-249: Remove duplicate line inclearDeleteState.The line
setDeleteDialogOpen(false)appears twice.const clearDeleteState = () => { setDeleteDialogOpen(false); setSelectedEntity(undefined); setSelectedId(undefined); - setDeleteDialogOpen(false); };
🧹 Nitpick comments (8)
src/domain/common/whiteboard/excalidraw/collab/Collab.ts (1)
328-335: Consider adding error handling for file loading failures.The Promise chain improves synchronization between elements and files rendering, reducing re-renders. However, file loading failures should be handled explicitly to ensure graceful degradation.
return this.filesManager.loadFiles({ files: remoteFiles }).then(() => { + // Return elements even if some files fail to load + return reconciledElements; +}).catch(error => { + logError('Failed to load remote files', { + category: TagCategoryValues.WHITEBOARD, + label: 'Collab', + error, + }); return reconciledElements; });src/domain/collaboration/whiteboard/containers/WhiteboardProvider.tsx (1)
41-49: Consider adding a default empty array for contributions.While the memoization implementation is good, the optional chaining on
callout?.contributionscould potentially result inundefined.find(). Consider providing a default empty array to make the code more robust.const whiteboardContribution = useMemo( () => - callout?.contributions?.find( + (callout?.contributions ?? []).find( contribution => contribution.whiteboard && (contribution.whiteboard.nameID === whiteboardId || contribution.whiteboard.id === whiteboardId) ), [callout, whiteboardId] );src/domain/common/whiteboard/excalidraw/useWhiteboardFilesManager.ts (1)
215-238: Consider batch processing for large file sets.While the current implementation with
Promise.allSettledhandles errors well, consider adding batch processing for better performance with large file sets.- await Promise.allSettled( - pendingFileIds.map(async fileId => { + const batchSize = 5; + for (let i = 0; i < pendingFileIds.length; i += batchSize) { + const batch = pendingFileIds.slice(i, i + batchSize); + await Promise.allSettled( + batch.map(async fileId => {src/domain/common/whiteboard/excalidraw/CollaborativeExcalidrawWrapper.tsx (2)
87-87: Consider moving the debounce interval constant.Move this constant closer to where it's used (near the
debouncedRefreshimplementation) to improve code organization.-const WINDOW_SCROLL_HANDLER_DEBOUNCE_INTERVAL = 100; const CollaborativeExcalidrawWrapper = ({ entities, actions, options, collabApiRef, children: renderChildren, }: WhiteboardWhiteboardProps) => { + const WINDOW_SCROLL_HANDLER_DEBOUNCE_INTERVAL = 100;
159-162: Enhance error logging message.While the error logging is good, consider adding more context to help with debugging.
- logError('WB Connection Closed', { + logError('Whiteboard: Collaboration connection closed unexpectedly', { category: TagCategoryValues.WHITEBOARD, - label: `WB ID: ${whiteboard?.id}; URL: ${whiteboard?.profile?.url}; Online: ${isOnline}`, + label: `Whiteboard ID: ${whiteboard?.id}; Profile URL: ${whiteboard?.profile?.url}; Online Status: ${isOnline}`,src/domain/community/contributor/Account/ContributorAccountView.tsx (3)
192-200: Optimize entitlement checks for better performance.Consider consolidating the entitlement checks into a single operation to avoid multiple array iterations.
-const isEntitledToCreateSpace = [ - LicenseEntitlementType.AccountSpaceFree, - LicenseEntitlementType.AccountSpacePlus, - LicenseEntitlementType.AccountSpacePremium, -].some(entitlement => myAccountEntitlements.includes(entitlement)); - -const isEntitledToCreateInnovationPack = myAccountEntitlements.includes(LicenseEntitlementType.AccountInnovationPack); -const isEntitledToCreateInnovationHub = myAccountEntitlements.includes(LicenseEntitlementType.AccountInnovationHub); -const isEntitledToCreateVC = myAccountEntitlements.includes(LicenseEntitlementType.AccountVirtualContributor); +const entitlements = { + space: [ + LicenseEntitlementType.AccountSpaceFree, + LicenseEntitlementType.AccountSpacePlus, + LicenseEntitlementType.AccountSpacePremium, + ], + innovationPack: [LicenseEntitlementType.AccountInnovationPack], + innovationHub: [LicenseEntitlementType.AccountInnovationHub], + virtualContributor: [LicenseEntitlementType.AccountVirtualContributor], +}; + +const isEntitled = useMemo(() => ({ + space: entitlements.space.some(e => myAccountEntitlements.includes(e)), + innovationPack: myAccountEntitlements.includes(entitlements.innovationPack[0]), + innovationHub: myAccountEntitlements.includes(entitlements.innovationHub[0]), + virtualContributor: myAccountEntitlements.includes(entitlements.virtualContributor[0]), +}), [myAccountEntitlements]);
259-271: Simplify deletion functions by removing redundant returns.The deletion functions have unnecessary return statements and similar patterns.
-const deleteSpace = () => { - if (!selectedId) { - return; - } - - return deleteSpaceMutation({ +const deleteSpace = () => + selectedId && deleteSpaceMutation({ variables: { input: { ID: selectedId, }, }, }); -};Apply similar pattern to
deleteVC,deletePack, anddeleteHubfunctions.Also applies to: 291-303, 323-333, 353-363
421-467: Consolidate entity action handlers to reduce duplication.The action handler functions follow similar patterns and could be consolidated into a single function.
+const getEntityActions = (id: string, entity: Entities, loading: boolean) => + canDeleteEntities && ( + <MenuItemWithIcon + key="delete" + disabled={loading} + iconComponent={DeleteOutline} + onClick={() => { + setSelectedEntity(entity); + setSelectedId(id); + setDeleteDialogOpen(true); + }} + > + {t('buttons.delete')} + </MenuItemWithIcon> + ); + -const getSpaceActions = (id: string) => - canDeleteEntities && ( - <MenuItemWithIcon - key="delete" - disabled={deleteSpaceLoading} - iconComponent={DeleteOutline} - onClick={() => onDeleteSpaceClick(id)} - > - {t('buttons.delete')} - </MenuItemWithIcon> - ); +const getSpaceActions = (id: string) => + getEntityActions(id, Entities.Space, deleteSpaceLoading);Apply similar pattern to
getVCActions,getPackActions, andgetHubActionsfunctions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
package.json(1 hunks)src/domain/collaboration/whiteboard/WhiteboardDialog/WhiteboardDialog.tsx(4 hunks)src/domain/collaboration/whiteboard/containers/WhiteboardProvider.tsx(2 hunks)src/domain/common/whiteboard/excalidraw/CollaborativeExcalidrawWrapper.tsx(3 hunks)src/domain/common/whiteboard/excalidraw/collab/Collab.ts(1 hunks)src/domain/common/whiteboard/excalidraw/useWhiteboardFilesManager.ts(1 hunks)src/domain/community/contributor/Account/ContributorAccountView.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.{ts,tsx,js}`: Review the React.js/TypeScript/JavaS...
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/collaboration/whiteboard/containers/WhiteboardProvider.tsxsrc/domain/common/whiteboard/excalidraw/useWhiteboardFilesManager.tssrc/domain/collaboration/whiteboard/WhiteboardDialog/WhiteboardDialog.tsxsrc/domain/common/whiteboard/excalidraw/collab/Collab.tssrc/domain/community/contributor/Account/ContributorAccountView.tsxsrc/domain/common/whiteboard/excalidraw/CollaborativeExcalidrawWrapper.tsx
🔇 Additional comments (8)
src/domain/collaboration/whiteboard/containers/WhiteboardProvider.tsx (1)
1-1: LGTM! Clean import addition.The useMemo import is correctly placed and properly destructured with other React imports.
src/domain/common/whiteboard/excalidraw/useWhiteboardFilesManager.ts (3)
204-209: LGTM! Efficient filtering of pending files.The optimization to filter out files that don't need downloading improves performance by avoiding unnecessary network requests.
222-235: LGTM! Robust error handling with proper logging.The error handling is well-implemented with:
- Descriptive error messages
- Proper use of Sentry for logging
- Appropriate error propagation
13-16: LGTM! Well-structured TypeScript interfaces.The TypeScript implementation is robust with:
- Clear and descriptive interfaces
- Proper type safety
- Effective use of generics
Also applies to: 63-86
src/domain/common/whiteboard/excalidraw/CollaborativeExcalidrawWrapper.tsx (2)
98-102: LGTM! Well-structured state management.The new state variables are well-typed and appropriately initialized for managing the Excalidraw API instance and collaboration status.
114-120: LGTM! Efficient scroll handling implementation.The memoized debounced scroll handler with proper cleanup improves performance by preventing unnecessary recreations.
src/domain/collaboration/whiteboard/WhiteboardDialog/WhiteboardDialog.tsx (2)
193-207: Well-implemented memoization with useCallback!The onClose function is properly memoized with all necessary dependencies, and the error handling is well-structured.
209-223: Excellent error handling with proper user feedback!The handleImportTemplate function is well-memoized and includes comprehensive error handling with both user notification and error logging.
src/domain/collaboration/whiteboard/WhiteboardDialog/WhiteboardDialog.tsx
Show resolved
Hide resolved
|
@bobbykolev: I'll pause the automated reviews for this PR. ✅ Actions performedReviews paused. |
Summary by CodeRabbit