Conversation
WalkthroughThe pull request introduces multiple changes across the Plane application, focusing on enhancing user interface components, improving state management, and refining user interactions. The modifications span various areas including workspace preferences, sticky notes, home widgets, and empty state components. Key changes include introducing new components like Changes
Suggested Labels
Suggested Reviewers
Poem
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
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
🧹 Nitpick comments (11)
web/core/components/stickies/delete-modal.tsx (1)
34-43: Consider adding a destructive variant to the modal.Since this is a delete operation, consider using a destructive variant of the modal to indicate the irreversible nature of the action.
<AlertModalCore handleClose={handleClose} handleSubmit={formSubmit} isSubmitting={loader} isOpen={isOpen} title="Delete sticky" + variant="destructive" content={<>Are you sure you want to delete the sticky? </>} />apiserver/plane/app/views/workspace/sticky.py (1)
47-53: Remove unnecessary parentheses and add error handling.The code has unnecessary parentheses and lacks error handling for database operations.
return self.paginate( request=request, - queryset=(stickies), + queryset=stickies, on_results=lambda stickies: StickySerializer(stickies, many=True).data, default_per_page=20, )Also, consider wrapping the database operation in a try-catch block to handle potential errors gracefully.
web/core/components/stickies/sticky/root.tsx (1)
Line range hint
47-52: Remove async/await as it's no longer needed.The
handleDeletefunction is marked as async but doesn't use any await operations.- const handleDelete = async () => { + const handleDelete = () => { if (!stickyId) return; onClose?.(); stickyOperations.remove(stickyId); };web/core/components/home/widgets/empty-states/recents.tsx (1)
4-14: Consider extracting common empty state styles.The implementation looks good with clear visual hierarchy and consistent styling. However, since we have multiple empty state components (recents and links), consider extracting common styles into shared classes or a base component.
Example shared styles that could be extracted:
+ // In a shared styles file or component + const emptyStateContainer = "flex w-full justify-center py-6 border-[1.5px] border-custom-border-100 rounded" + const emptyStateIconWrapper = "mb-2 rounded-full mx-auto last:rounded-full w-[50px] h-[50px] flex items-center justify-center bg-custom-background-80/40 transition-transform duration-300"web/core/components/home/widgets/empty-states/links.tsx (2)
19-20: Remove unnecessary string concatenation.The empty space concatenation using
{" "}is unnecessary here since it's at the end of the text.- Add any links you need for quick access to your work.{" "} + Add any links you need for quick access to your work.
21-23: Consider extracting button text to constants.The implementation looks good, but consider extracting the button text to a constants file for better maintainability and potential internationalization.
// constants/empty-states.ts export const EMPTY_STATE_ACTIONS = { ADD_QUICK_LINK: "Add quick link" };web/core/components/home/widgets/links/links.tsx (1)
32-45: Optimize ContentOverflowWrapper implementation.The implementation looks good, but there are a few improvements possible:
- The empty Fragment in the fallback prop is unnecessary
- The nested div structure could be flattened
Consider this optimization:
<ContentOverflowWrapper maxHeight={150} containerClassName="pb-2 box-border" - fallback={<></>} + fallback={null} buttonClassName="bg-custom-background-90/20" > - <div> <div className="flex gap-2 mb-2 flex-wrap"> {links && links.length > 0 && links.map((linkId) => <ProjectLinkDetail key={linkId} linkId={linkId} linkOperations={linkOperations} />)} </div> - </div> </ContentOverflowWrapper>🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: 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.
(lint/complexity/noUselessFragments)
web/core/components/core/content-overflow-HOC.tsx (2)
30-71: Add error handling for observer setup.While the observer setup is comprehensive, it lacks error handling for browsers that don't support ResizeObserver or MutationObserver.
Consider adding feature detection and fallback:
useEffect(() => { if (!contentRef?.current) return; + + // Feature detection + if (typeof ResizeObserver === 'undefined' || typeof MutationObserver === 'undefined') { + console.warn('Browser does not support required features for ContentOverflowWrapper'); + updateHeight(); + return; + } const updateHeight = () => { if (contentRef.current) { const height = contentRef.current.getBoundingClientRect().height; setContainerHeight(height); } }; // ... rest of the code
89-108: Consider performance optimization for gradient effect.The gradient effect implementation might cause performance issues on scroll. Consider using CSS
position: stickyfor the button container.Here's an optimized version:
{containerHeight > maxHeight && ( <div className={cn( - "bottom-0 left-0 w-full", + "sticky bottom-0 left-0 w-full", `bg-gradient-to-t from-custom-background-100 to-transparent flex flex-col items-center justify-end`, "text-center", { - "absolute h-[100px]": !showAll, + "h-[100px]": !showAll, "h-[30px]": showAll, } )} >web/core/components/home/widgets/recents/index.tsx (1)
Line range hint
82-93: Consolidate height definitions and improve accessibility.The component has duplicate height definitions and could benefit from accessibility improvements.
Consider these improvements:
- <div ref={ref} className=" max-h-[500px] min-h-[250px] overflow-y-scroll"> + <div + ref={ref} + className="max-h-[500px] min-h-[250px] overflow-y-auto" + role="region" + aria-label="Recent activities" + >Note: Changed
overflow-y-scrolltooverflow-y-autoto prevent unnecessary scrollbars.web/core/components/stickies/stickies-layout.tsx (1)
113-113: Remove unnecessary Fragment.The empty Fragment used as fallback can be simplified to
null.- fallback={<></>} + fallback={null}🧰 Tools
🪛 Biome (1.9.4)
[error] 113-113: 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.
(lint/complexity/noUselessFragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apiserver/plane/app/views/workspace/preference.py(1 hunks)apiserver/plane/app/views/workspace/sticky.py(1 hunks)web/core/components/core/content-overflow-HOC.tsx(1 hunks)web/core/components/editor/sticky-editor/editor.tsx(1 hunks)web/core/components/home/widgets/empty-states/issues.tsx(0 hunks)web/core/components/home/widgets/empty-states/links.tsx(1 hunks)web/core/components/home/widgets/empty-states/recents.tsx(1 hunks)web/core/components/home/widgets/links/links.tsx(2 hunks)web/core/components/home/widgets/links/root.tsx(2 hunks)web/core/components/home/widgets/loaders/quick-links.tsx(1 hunks)web/core/components/home/widgets/loaders/recent-activity.tsx(1 hunks)web/core/components/home/widgets/recents/index.tsx(2 hunks)web/core/components/stickies/delete-modal.tsx(1 hunks)web/core/components/stickies/empty.tsx(3 hunks)web/core/components/stickies/stickies-layout.tsx(2 hunks)web/core/components/stickies/sticky/inputs.tsx(1 hunks)web/core/components/stickies/sticky/root.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- web/core/components/home/widgets/empty-states/issues.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
web/core/components/stickies/stickies-layout.tsx
[error] 113-113: 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.
(lint/complexity/noUselessFragments)
web/core/components/stickies/delete-modal.tsx
[error] 23-29: This code is unreachable
(lint/correctness/noUnreachable)
web/core/components/home/widgets/links/links.tsx
[error] 35-35: 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.
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint-web
- GitHub Check: lint-apiserver
- GitHub Check: Analyze (javascript)
🔇 Additional comments (13)
web/core/components/stickies/empty.tsx (1)
Line range hint
11-33: LGTM! Well-structured empty state implementation.The component has good UX considerations:
- Clear visual hierarchy with icon, title, and description
- Loading state feedback during sticky creation
- Proper use of semantic HTML and accessibility attributes
web/core/components/stickies/sticky/root.tsx (1)
Line range hint
38-43: Fix dependency array in debounced callback.The
debouncedFormSavecallback depends onstickyDatabut might use stale values due to closure. Consider using the callback form of setState or moving the dependency to the payload.const debouncedFormSave = useCallback( debounce(async (payload: Partial<TSticky>) => { await handleChange(payload); }, 500), - [stickyOperations, stickyData, handleChange] + [handleChange] );✅ Verification successful
Fix dependency array in debounced callback.
The review comment is correct. The
debouncedFormSavecallback only useshandleChangeand passes the payload directly to it. SincehandleChangealready includesstickyOperationsin its own dependency array andstickyDatais not used within the callback, the dependency array should only includehandleChange.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for similar patterns in the codebase that might have the same issue ast-grep --pattern 'debounce($_, $_)'Length of output: 2282
Script:
#!/bin/bash # Get the complete implementation of the sticky root component ast-grep --pattern 'const debouncedFormSave = useCallback($$$)' -A 20 rg -A 10 "const handleChange = " web/core/components/stickies/sticky/root.tsxLength of output: 4830
apiserver/plane/app/views/workspace/preference.py (1)
34-38: Document the rationale for excluding specific preference keys.While the implementation is correct, please add a comment explaining why "quick_tutorial" and "new_at_plane" keys are being excluded from workspace preferences.
Also, consider how this change affects existing preferences with these keys.
web/core/components/editor/sticky-editor/editor.tsx (1)
32-32: Document the new deletion flow architecture.The change to
handleDeletesignature suggests a refactoring of the deletion flow. Please:
- Document the new deletion architecture in the PR description
- Explain how async operations are now handled
web/core/components/home/widgets/loaders/quick-links.tsx (1)
8-8: LGTM! Layout classes are well structured.The flex layout without justify-center allows for natural distribution of items, which works well with the flex-wrap behavior.
web/core/components/home/widgets/loaders/recent-activity.tsx (1)
9-9: Verify alignment with actual data display.The reduction from 7 to 5 loader items seems intentional. However, let's verify this aligns with the actual number of items typically displayed in the recent activity list.
✅ Verification successful
Loader item count aligns with widget design
The reduction to 5 loader items is appropriate as:
- The widget shows a preview of activities with a "View all" link
- The change is coordinated with empty state adjustments
- The actual number of items is controlled by the backend API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the recent activity list implementation to verify typical item count ast-grep --pattern 'const RecentActivityWidget = () => { $$$ return ( $$$ {items.map((item) => ( $$$ ))} $$$ ) }'Length of output: 143
Script:
#!/bin/bash # Search for activity-related files fd "activity" --type f # Search for activity-related components with different naming patterns ast-grep --pattern 'const $_Activity = () => { $$$ }' # Look for limit or pagination configurations rg -i "limit.*activity|per.*page.*activity" -A 2Length of output: 2557
Script:
#!/bin/bash # Check the actual recent activity widget implementation cat web/core/components/dashboard/widgets/recent-activity.tsx # Look for any default limits or slice operations rg -A 3 "slice\(" web/core/components/dashboard/widgets/recent-activity.tsxLength of output: 4454
Script:
#!/bin/bash # Search for the recent activity API endpoint implementation rg -l "recent_activity" apiserver/plane/app/views/ rg -A 5 "def get" $(rg -l "recent_activity" apiserver/plane/app/views/)Length of output: 524
web/core/components/home/widgets/empty-states/links.tsx (1)
4-27: LGTM! Well-structured empty state component.The component is well implemented with proper typing, clear visual hierarchy, and good user interaction through the create button.
web/core/components/home/widgets/links/links.tsx (1)
30-30: LGTM! Improved empty state handling.The dedicated
LinksEmptyStatecomponent with create action improves the user experience.web/core/components/home/widgets/links/root.tsx (1)
35-50: LGTM! Well-structured UI with clear hierarchy.The layout improvements with proper spacing and semantic structure enhance usability. The "Add quick link" button is well-positioned and styled consistently.
web/core/components/home/widgets/recents/index.tsx (1)
Line range hint
88-93: Verify loading state handling.The loading state might cause layout shifts. Consider maintaining consistent height during loading.
web/core/components/stickies/stickies-layout.tsx (3)
14-14: LGTM!The import of
ContentOverflowWrapperfollows the project's conventions.
109-118: Great architectural improvement!Delegating overflow handling to a dedicated
ContentOverflowWrappercomponent reduces complexity and improves maintainability by removing manual height management logic.🧰 Tools
🪛 Biome (1.9.4)
[error] 113-113: 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.
(lint/complexity/noUselessFragments)
117-117: Investigate and fix the TypeScript type mismatch.Instead of suppressing the TypeScript error with
@ts-expect-error, please investigate and fix the type mismatch in the Masonry component. This will improve type safety and prevent potential runtime issues.Let's check the Masonry component's type definitions:
Would you like me to help investigate and propose a fix for the type mismatch?
| const formSubmit = async () => { | ||
| await handleSubmit(); | ||
| setLoader(true); | ||
| try { | ||
| } catch (error) { | ||
| setToast({ | ||
| type: TOAST_TYPE.ERROR, | ||
| title: "Warning!", | ||
| message: "Something went wrong please try again later.", | ||
| }); | ||
| } | ||
|
|
||
| setLoader(false); | ||
| }; |
There was a problem hiding this comment.
Fix the error handling logic in formSubmit.
The current implementation has several issues:
- The try block is empty, making the catch block unreachable
- The loader state is set after the async operation starts
- The error message is generic
Apply this diff to fix the issues:
const formSubmit = async () => {
- await handleSubmit();
- setLoader(true);
- try {
- } catch (error) {
+ setLoader(true);
+ try {
+ await handleSubmit();
+ } catch (error) {
setToast({
type: TOAST_TYPE.ERROR,
- title: "Warning!",
- message: "Something went wrong please try again later.",
+ title: "Error",
+ message: error instanceof Error ? error.message : "Failed to delete sticky note",
});
+ throw error;
}
-
- setLoader(false);
+ finally {
+ setLoader(false);
+ }
};📝 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.
| const formSubmit = async () => { | |
| await handleSubmit(); | |
| setLoader(true); | |
| try { | |
| } catch (error) { | |
| setToast({ | |
| type: TOAST_TYPE.ERROR, | |
| title: "Warning!", | |
| message: "Something went wrong please try again later.", | |
| }); | |
| } | |
| setLoader(false); | |
| }; | |
| const formSubmit = async () => { | |
| setLoader(true); | |
| try { | |
| await handleSubmit(); | |
| } catch (error) { | |
| setToast({ | |
| type: TOAST_TYPE.ERROR, | |
| title: "Error", | |
| message: error instanceof Error ? error.message : "Failed to delete sticky note", | |
| }); | |
| throw error; | |
| } | |
| finally { | |
| setLoader(false); | |
| } | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 23-29: This code is unreachable
(lint/correctness/noUnreachable)
| stickyId: string | undefined; | ||
| handleChange: (data: Partial<TSticky>) => Promise<void>; | ||
| handleDelete: () => Promise<void>; | ||
| handleDelete: () => void; |
There was a problem hiding this comment.
💡 Codebase verification
Breaking Change Confirmed: Type mismatch between implementation and interface
The type change from Promise<void> to void conflicts with the actual implementation in sticky/root.tsx where handleDelete is an async function. This will cause TypeScript errors and potential runtime issues.
web/core/components/stickies/sticky/root.tsx: Implementation isasyncweb/core/components/stickies/sticky/inputs.tsx: Type isvoid
🔗 Analysis chain
Breaking Change: Document handleDelete signature change.
The return type change from Promise<void> to void is a breaking change. Please:
- Document this change in the PR description
- Ensure all implementations are updated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for handleDelete implementations that might need updates
echo "Searching for handleDelete implementations..."
rg "handleDelete.*Promise" --type typescript
Length of output: 182
Script:
#!/bin/bash
# Search for handleDelete implementations and type declarations
echo "Searching for handleDelete in method implementations..."
ast-grep --pattern 'handleDelete($$$) { $$ }'
echo -e "\nSearching for handleDelete in type declarations..."
ast-grep --pattern 'handleDelete: $$$'
echo -e "\nSearching for handleDelete with Promise..."
rg "handleDelete.*Promise" -t ts -t tsx
Length of output: 497
Script:
#!/bin/bash
# Search for any handleDelete occurrences with context
echo "Searching for handleDelete patterns..."
rg "handleDelete" --type-add 'ts:*.{ts,tsx}' --type ts -C 2
Length of output: 39561
rahulramesha
left a comment
rahulramesha
left a comment
There was a problem hiding this comment.
Please check linting as well
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/core/components/stickies/stickies-layout.tsx (1)
110-115: Enhance ContentOverflowWrapper configuration.A few suggestions to improve the code:
- The empty fragment as fallback seems unnecessary
- Consider extracting the magic number 650 as a named constant for better maintainability
+const STICKY_CONTAINER_MAX_HEIGHT = 650; + export const StickyAll = observer((props: TProps) => { // ... return ( <div ref={containerRef}> <ContentOverflowWrapper - maxHeight={650} + maxHeight={STICKY_CONTAINER_MAX_HEIGHT} containerClassName="pb-2 box-border" - fallback={<></>} + fallback={null} buttonClassName="bg-custom-background-90/20" >🧰 Tools
🪛 Biome (1.9.4)
[error] 113-113: 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.
(lint/complexity/noUselessFragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/core/components/stickies/delete-modal.tsx(1 hunks)web/core/components/stickies/stickies-layout.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/components/stickies/delete-modal.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
web/core/components/stickies/stickies-layout.tsx
[error] 113-113: 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.
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint-web
- GitHub Check: lint-apiserver
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
web/core/components/stickies/stickies-layout.tsx (4)
10-10: LGTM! Import of ContentOverflowWrapper aligns with PR objectives.The addition of ContentOverflowWrapper is consistent with the PR's goal of implementing content overflow management for widgets.
Line range hint
144-156: LGTM! Well-implemented responsive layout.The responsive implementation is well-structured with:
- Clear breakpoint definitions
- Proper ResizeObserver usage with cleanup
- Good fallback handling
Line range hint
1-1: Verify search functionality implementation.The PR objectives mention adding search functionality for workspace stickies, but this file doesn't show the implementation. Please ensure the search feature is implemented in other files or update the PR description.
Let's check for search-related changes:
✅ Verification successful
Search functionality is properly implemented ✓
The search feature for workspace stickies is implemented in
web/core/store/sticky/sticky.store.ts. ThegetWorkspaceStickiesmethod includes case-insensitive filtering of stickies based on their names using the store's search query.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for files containing sticky search implementation rg -g '!{dist,node_modules}/**' -l 'search.*stick|stick.*search'Length of output: 104
Script:
#!/bin/bash # Check the content of sticky store for search implementation rg -A 5 -B 5 'search.*stick|stick.*search' web/core/store/sticky/sticky.store.tsLength of output: 523
117-117: Address the type mismatch with Masonry component.The
@ts-expect-errorcomment suggests a type mismatch with the Masonry component. Consider fixing the type issue instead of suppressing it to ensure type safety.Let's check the Masonry component's type definition:
5 participants
Summary by CodeRabbit
Based on the provided summary, here are the release notes:
New Features
Improvements
Bug Fixes
Chores
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements
Bug Fixes
Removed Components
IssuesEmptyStatecomponent