[WEB-3045] fix: stickies bugs #6433
Conversation
WalkthroughThe pull request introduces several modifications to the stickies components across multiple files. The changes focus on enhancing the loading state, layout management, and error handling in the stickies functionality. A new Changes
Sequence DiagramsequenceDiagram
participant User
participant StickiesList
participant StickyDNDWrapper
participant StickyNote
participant HandleLayout
User->>StickiesList: Interact with Stickies
StickiesList->>StickyDNDWrapper: Trigger Layout Update
StickyDNDWrapper->>StickyNote: Pass handleLayout
StickyNote->>HandleLayout: Call Layout Update
HandleLayout-->>StickyNote: Reflow Layout
Suggested reviewers
Poem
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: 3
🔭 Outside diff range comments (1)
web/core/components/stickies/sticky/inputs.tsx (1)
Line range hint
65-77: Implement proper file upload handling.The current implementation of
uploadFilereturns an empty string and lacks error handling. This could lead to silent failures and poor user experience.Consider implementing proper file upload handling:
- uploadFile={async () => ""} + uploadFile={async (file: File) => { + try { + // Implement actual file upload logic + const uploadedUrl = await yourFileUploadService.upload(file); + return uploadedUrl; + } catch (error) { + console.error("File upload failed:", error); + throw new Error("Failed to upload file. Please try again."); + } + }}
🧹 Nitpick comments (5)
web/core/components/stickies/sticky/inputs.tsx (1)
Line range hint
37-44: Consider debouncing editor updates for better performance.The form submission is triggered on every editor change, which could lead to excessive updates. Consider implementing debouncing to optimize performance.
+ import { debounce } from "lodash"; const handleFormSubmit = useCallback( - async (formdata: Partial<TSticky>) => { - await handleUpdate({ + debounce((formdata: Partial<TSticky>) => { + handleUpdate({ description_html: formdata.description_html ?? "<p></p>", - }); + }); + }, 300), }, [handleUpdate] );web/core/components/stickies/delete-modal.tsx (1)
23-28: Consider logging the error for debugging.The error catch block discards the error information. Consider logging it for debugging purposes.
- } catch { + } catch (error) { + console.error("Failed to delete sticky:", error); setToast({ type: TOAST_TYPE.ERROR, title: "Warning!", message: "Something went wrong. Please try again later.", });web/core/components/stickies/layout/stickies-loader.tsx (1)
5-44: LGTM! Well-structured loading skeleton.The loader component effectively mimics the sticky note structure with appropriate spacing and layout. Good use of grid and flex layouts for responsive design.
A minor optimization suggestion: Consider extracting the repeated loader structure into a separate component for better maintainability.
const StickyLoader = () => ( <Loader className="space-y-5 border border-custom-border-200 p-3 rounded"> {/* ... existing loader structure ... */} </Loader> ); export const StickiesLoader = () => ( <div className="overflow-scroll pb-2 grid grid-cols-4 gap-4"> {Array.from({ length: 4 }).map((_, index) => ( <StickyLoader key={index} /> ))} </div> );web/core/components/stickies/layout/stickies-list.tsx (2)
44-51: Optimize layout handling with useCallback.The
handleLayoutfunction should be memoized to prevent unnecessary re-renders of child components.- const handleLayout = () => { + const handleLayout = useCallback(() => { if (masonryRef.current) { // Force reflow masonryRef.current.performLayout(); } - }; + }, []);
Line range hint
88-102: Extract empty state logic to a separate component.The empty state conditional rendering is becoming complex and could benefit from being extracted into a dedicated component.
Consider creating a new component like this:
type StickiesEmptyStateProps = { isStickiesPage: boolean; searchQuery: string; onCreateSticky: () => void; }; const StickiesEmptyStateWrapper: React.FC<StickiesEmptyStateProps> = ({ isStickiesPage, searchQuery, onCreateSticky, }) => { if (isStickiesPage || searchQuery) { return ( <EmptyState type={searchQuery ? EmptyStateType.STICKIES_SEARCH : EmptyStateType.STICKIES} layout={searchQuery ? "screen-simple" : "screen-detailed"} primaryButtonOnClick={onCreateSticky} primaryButtonConfig={{ size: "sm" }} /> ); } return <StickiesEmptyState />; };This would simplify the main component and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
web/core/components/stickies/delete-modal.tsx(3 hunks)web/core/components/stickies/layout/stickies-list.tsx(5 hunks)web/core/components/stickies/layout/stickies-loader.tsx(1 hunks)web/core/components/stickies/layout/sticky-dnd-wrapper.tsx(1 hunks)web/core/components/stickies/modal/stickies.tsx(3 hunks)web/core/components/stickies/sticky/inputs.tsx(1 hunks)web/core/components/stickies/sticky/root.tsx(4 hunks)
🔇 Additional comments (7)
web/core/components/stickies/sticky/inputs.tsx (1)
Line range hint
15-19: Clarify the distinction betweenhandleUpdateandhandleChange.The props have similar signatures but different async behaviors:
handleUpdate: synchronous, returns voidhandleChange: asynchronous, returns PromiseThis inconsistency could lead to confusion. Consider:
- Renaming them to better reflect their specific purposes
- Maintaining consistent async patterns
Let's verify the usage pattern of these handlers:
web/core/components/stickies/delete-modal.tsx (1)
10-10: LGTM! Good async handling.The
handleSubmitmethod is now properly typed as async, which better reflects its asynchronous nature.web/core/components/stickies/modal/stickies.tsx (2)
31-33: LGTM! Improved visual hierarchy.Good improvements to the title section with better text contrast and icon placement.
Line range hint
59-64: LGTM! Added semantic button type.Good addition of
type="button"attribute for better semantics.web/core/components/stickies/sticky/root.tsx (2)
23-23: LGTM! Good prop addition.The
handleLayoutprop is well integrated into the component's type definition and props destructuring.Also applies to: 26-26
91-94: LGTM! Good update handling.The update handler properly chains the layout update with the form save operation.
web/core/components/stickies/layout/sticky-dnd-wrapper.tsx (1)
27-36: Props type definition looks good but verify DND removal requirement.The Props type is well-defined with proper TypeScript types. However, there seems to be an inconsistency with the PR objectives which mention DND removal, yet this component retains all drag-and-drop functionality.
Could you clarify if the DND functionality should be removed as mentioned in the PR objectives? If so, this component would need significant refactoring.
| const { onClose, workspaceSlug, className = "", stickyId, showToolbar, handleLayout } = props; | ||
| // navigation | ||
| const pathName = usePathname(); | ||
| // const pathName = usePathname(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented code.
There are several instances of commented-out code that should be removed to maintain code cleanliness:
- Line 28:
// const pathName = usePathname(); - Line 37:
// const isStickiesPage = pathName?.includes("stickies"); - Line 81:
// {isStickiesPage && <StickyItemDragHandle isDragging={false} />}{" "}
- // const pathName = usePathname();
- // const isStickiesPage = pathName?.includes("stickies");
- // {isStickiesPage && <StickyItemDragHandle isDragging={false} />}{" "}Also applies to: 37-37, 81-81
| render: ({ container }) => { | ||
| const root = createRoot(container); | ||
| root.render( | ||
| <div className="scale-50"> | ||
| <div className="-m-2 max-h-[150px]"> | ||
| <StickyNote | ||
| className={"w-[290px]"} | ||
| workspaceSlug={workspaceSlug.toString()} | ||
| stickyId={stickyId} | ||
| showToolbar={false} | ||
| /> | ||
| </div> | ||
| ); | ||
| return () => root.unmount(); | ||
| }, | ||
| nativeSetDragImage, | ||
| }); | ||
| }, | ||
| }), | ||
| dropTargetForElements({ | ||
| element, | ||
| canDrop: ({ source }) => source.data?.type === "sticky", | ||
| getData: ({ input, element }) => { | ||
| const blockedStates: InstructionType[] = ["make-child"]; | ||
| if (!isLastChild) { | ||
| blockedStates.push("reorder-below"); | ||
| } | ||
| </div> | ||
| ); | ||
| return () => root.unmount(); | ||
| }, |
There was a problem hiding this comment.
Fix potential memory leak in drag preview rendering.
The drag preview rendering using createRoot might lead to memory leaks. While the root is unmounted in the cleanup function, any resources created within the preview (like event listeners) might not be properly cleaned up.
Consider refactoring to:
- Move the preview component to a separate file
- Ensure all resources are cleaned up properly
- Use
useCallbackfor the render function to prevent unnecessary re-renders
- render: ({ container }) => {
- const root = createRoot(container);
- root.render(
- <div className="scale-50">
- <div className="-m-2 max-h-[150px]">
- <StickyNote
- className={"w-[290px]"}
- workspaceSlug={workspaceSlug.toString()}
- stickyId={stickyId}
- showToolbar={false}
- />
- </div>
- </div>
- );
- return () => root.unmount();
- },
+ render: useCallback(({ container }) => {
+ const root = createRoot(container);
+ const Preview = () => (
+ <div className="scale-50">
+ <div className="-m-2 max-h-[150px]">
+ <StickyNote
+ className={"w-[290px]"}
+ workspaceSlug={workspaceSlug.toString()}
+ stickyId={stickyId}
+ showToolbar={false}
+ />
+ </div>
+ </div>
+ );
+ root.render(<Preview />);
+ return () => {
+ root.unmount();
+ // Add any additional cleanup here
+ };
+ }, [workspaceSlug, stickyId]),📝 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.
| render: ({ container }) => { | |
| const root = createRoot(container); | |
| root.render( | |
| <div className="scale-50"> | |
| <div className="-m-2 max-h-[150px]"> | |
| <StickyNote | |
| className={"w-[290px]"} | |
| workspaceSlug={workspaceSlug.toString()} | |
| stickyId={stickyId} | |
| showToolbar={false} | |
| /> | |
| </div> | |
| ); | |
| return () => root.unmount(); | |
| }, | |
| nativeSetDragImage, | |
| }); | |
| }, | |
| }), | |
| dropTargetForElements({ | |
| element, | |
| canDrop: ({ source }) => source.data?.type === "sticky", | |
| getData: ({ input, element }) => { | |
| const blockedStates: InstructionType[] = ["make-child"]; | |
| if (!isLastChild) { | |
| blockedStates.push("reorder-below"); | |
| } | |
| </div> | |
| ); | |
| return () => root.unmount(); | |
| }, | |
| render: useCallback(({ container }) => { | |
| const root = createRoot(container); | |
| const Preview = () => ( | |
| <div className="scale-50"> | |
| <div className="-m-2 max-h-[150px]"> | |
| <StickyNote | |
| className={"w-[290px]"} | |
| workspaceSlug={workspaceSlug.toString()} | |
| stickyId={stickyId} | |
| showToolbar={false} | |
| /> | |
| </div> | |
| </div> | |
| ); | |
| root.render(<Preview />); | |
| return () => { | |
| root.unmount(); | |
| // Add any additional cleanup here | |
| }; | |
| }, [workspaceSlug, stickyId]), |
| {/* @ts-expect-error type mismatch here */} | ||
| <Masonry elementType="div"> | ||
| <Masonry elementType="div" ref={masonryRef}> |
There was a problem hiding this comment.
Fix type mismatch in Masonry component.
The @ts-expect-error comment indicates a known type issue that should be properly addressed rather than suppressed.
Please investigate the type mismatch and consider:
- Updating the type definitions for the Masonry component
- Using a properly typed alternative
- Creating a proper type declaration file if one doesn't exist
| const handleLayout = () => { | ||
| if (masonryRef.current) { | ||
| // Force reflow | ||
| masonryRef.current.performLayout(); |
There was a problem hiding this comment.
@aaryan610 could you please let me know what is happening here? cant find mdn docs for it? going by your comment are you trying to force layout reflow? if that is the case it is usually not a good practice since it is better to always allow browser decide on forcing layout reflow
There was a problem hiding this comment.
If the content addition in sticky was resulting in increment of height then the masonry library wasn't adjusting the layout, resulting in overlap of stickies. Therefore i am doing a force update.
Description
This PR brings the following bug fixes and improvements to stickies-
Type of Change
Summary by CodeRabbit
New Features
StickiesLoadercomponent to improve loading state visualizationBug Fixes
Style
Refactor