-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WEB-3048] feat: added-stickies #6339
Conversation
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces significant enhancements to the sticky notes functionality across multiple components and services. It includes the addition of new types, services, stores, hooks, and UI components for managing sticky notes within a workspace. The changes enable features for creating, updating, deleting, and displaying sticky notes, with a focus on improving the user experience and interaction. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
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 (
|
…into feat-ce-stickies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 17
♻️ Duplicate comments (1)
web/core/components/editor/sticky-editor/toolbar.tsx (1)
40-42
:⚠️ Potential issueAddress TODO comment and type assertion.
Similar to the editor component, there's a TODO comment about toolbar homogenization and a type assertion that needs attention.
This is a duplicate of the issue found in the editor component. The fix should be coordinated between both files.
🧹 Nitpick comments (33)
web/core/store/sticky/sticky.store.ts (2)
73-73
: Simplify conditional with optional chainingIn line 73, you can simplify the conditional check by using optional chaining. This enhances readability and handles cases where
sticky.name
might beundefined
ornull
.Apply this diff to simplify the condition:
- (sticky) => sticky.name && sticky.name.toLowerCase().includes(this.searchQuery.toLowerCase()) + (sticky) => sticky.name?.toLowerCase().includes(this.searchQuery.toLowerCase())🧰 Tools
🪛 Biome (1.9.4)
[error] 73-73: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
137-152
: Ensure state consistency inupdateSticky
methodIn the
updateSticky
method, if an error occurs during thestickyService.updateSticky
call, the state of the sticky is reverted. However, consider also handling thecreatingSticky
state and providing user feedback in case of errors.You might want to update the catch block to handle any loading indicators and inform the user about the failure.
web/core/hooks/use-stickies.tsx (1)
7-11
: LGTM! Consider enhancing error handling.The hook implementation follows React best practices. For better debugging, consider adding the component name to the error message.
- if (context === undefined) throw new Error("useSticky must be used within StoreProvider"); + if (context === undefined) { + const error = new Error("useSticky must be used within StoreProvider"); + error.name = "StickyStoreError"; + throw error; + }web/core/components/stickies/modal/index.tsx (1)
8-15
: Enhance modal accessibility.The modal implementation looks good but could benefit from accessibility improvements.
export const AllStickiesModal = (props: TProps) => { const { isOpen, handleClose } = props; return ( - <ModalCore isOpen={isOpen} handleClose={handleClose} width={EModalWidth.VXL}> + <ModalCore + isOpen={isOpen} + handleClose={handleClose} + width={EModalWidth.VXL} + aria-labelledby="stickies-modal-title" + role="dialog" + > + <h2 id="stickies-modal-title" className="sr-only">All Stickies</h2> <Stickies handleClose={handleClose} /> </ModalCore> ); };web/core/components/home/widgets/empty-states/issues.tsx (3)
12-12
: Address TODO comment about general component.The TODO suggests creating a general empty state component. This would improve reusability and consistency across the application.
Would you like me to help create a general empty state component that can be reused across different widgets?
16-16
: Improve image accessibility.The alt text "Assigned issues" could be more descriptive for better accessibility. Consider using "No assigned issues found" or "Empty state for assigned issues".
- <Image src={image} className="w-full h-full" alt="Assigned issues" /> + <Image src={image} className="w-full h-full" alt="No assigned issues found" />
18-18
: Simplify text styling.The
whitespace-pre-line
class might be unnecessary for single-line text.- <p className="text-sm font-medium text-custom-text-300 whitespace-pre-line">No activity to display</p> + <p className="text-sm font-medium text-custom-text-300">No activity to display</p>web/core/components/editor/sticky-editor/color-pallete.tsx (2)
3-12
: Consider using a const enum or object for colors.The color array could be more maintainable and type-safe using a const enum or object with named keys.
-export const STICKY_COLORS = [ - "#D4DEF7", // light periwinkle - "#B4E4FF", // light blue - "#FFF2B4", // light yellow - "#E3E3E3", // light gray - "#FFE2DD", // light pink - "#F5D1A5", // light orange - "#D1F7C4", // light green - "#E5D4FF", // light purple -]; +export const STICKY_COLORS = { + PERIWINKLE: "#D4DEF7", + BLUE: "#B4E4FF", + YELLOW: "#FFF2B4", + GRAY: "#E3E3E3", + PINK: "#FFE2DD", + ORANGE: "#F5D1A5", + GREEN: "#D1F7C4", + PURPLE: "#E5D4FF", +} as const;
1-36
: Fix filename typo.The filename contains a typo: "pallete" should be "palette".
Please rename the file from
color-pallete.tsx
tocolor-palette.tsx
.web/core/components/stickies/empty.tsx (2)
3-6
: Props interface could be more descriptiveConsider renaming
TProps
to something more specific likeEmptyStateProps
for better code readability and documentation.-type TProps = { +interface EmptyStateProps = { handleCreate: () => void; creatingSticky?: boolean; -}; +}
27-35
: Enhance loading state accessibilityThe loading spinner could benefit from a more descriptive aria-label and additional ARIA attributes.
<div className={`w-4 h-4 border-2 border-t-transparent rounded-full animate-spin border-custom-primary-100`} role="status" - aria-label="loading" + aria-label="Creating new sticky note" + aria-live="polite" + aria-busy="true" />web/core/components/stickies/widget.tsx (1)
17-20
: Optimize event handler performanceMove the click handler outside the render function to prevent unnecessary re-creations.
+const handleAddSticky = useCallback(() => { + toggleShowNewSticky(true); + stickyOperations.create({ color: STICKY_COLORS[0] }); +}, [toggleShowNewSticky, stickyOperations]); -onClick={() => { - toggleShowNewSticky(true); - stickyOperations.create({ color: STICKY_COLORS[0] }); -}} +onClick={handleAddSticky}web/core/services/sticky.service.ts (2)
20-35
: Enhance pagination implementationThe pagination implementation has hardcoded values and could be more flexible:
- Consider making the default values configurable
- Add type safety for cursor format
- Document the pagination structure
+const DEFAULT_PAGE_SIZE = 5; +const DEFAULT_CURSOR = '5:0:0'; + +interface PaginationParams { + cursor?: string; + per_page?: number; +} + async getStickies( workspaceSlug: string, - cursor?: string, - per_page?: number + { cursor, per_page }: PaginationParams = {} ): Promise<{ results: TSticky[]; total_pages: number }> { return this.get(`/api/workspaces/${workspaceSlug}/stickies/`, { params: { - cursor: cursor || `5:0:0`, - per_page: per_page || 5, + cursor: cursor || DEFAULT_CURSOR, + per_page: per_page || DEFAULT_PAGE_SIZE, }, })
12-18
: Improve error handling type safetyThe error handling could be more specific with custom error types.
+interface StickyServiceError { + message: string; + code: string; + details?: Record<string, string[]>; +} + async createSticky(workspaceSlug: string, payload: Partial<TSticky>) { return this.post(`/api/workspaces/${workspaceSlug}/stickies/`, payload) .then((res) => res?.data) .catch((err) => { - throw err?.response?.data; + throw err?.response?.data as StickyServiceError; }); }web/core/components/home/home-dashboard-widgets.tsx (2)
41-50
: Optimize widget rendering performanceConsider memoizing the widget components to prevent unnecessary re-renders.
+const MemoizedWidget = memo(({ component: WidgetComponent, workspaceSlug }: { + component: React.FC<THomeWidgetProps>; + workspaceSlug: string; +}) => ( + <WidgetComponent workspaceSlug={workspaceSlug} /> +)); {orderedWidgets.map((key) => { const WidgetComponent = WIDGETS_LIST[key]?.component; const isEnabled = widgetsMap[key]?.is_enabled; if (!WidgetComponent || !isEnabled) return null; return ( <div key={key} className="py-4"> - <WidgetComponent workspaceSlug={workspaceSlug.toString()} /> + <MemoizedWidget component={WidgetComponent} workspaceSlug={workspaceSlug.toString()} /> </div> ); })}
Line range hint
13-19
: Consider extracting widget configurationThe widget configuration could be moved to a separate file for better maintainability.
Consider creating a new file
constants/widgets.ts
to store the widget configuration.web/core/components/stickies/sticky/root.tsx (1)
38-43
: Optimize debounce dependencies.The dependencies array for
debouncedFormSave
includesstickyOperations
andstickyData
, but these aren't used in the debounced function. OnlyhandleChange
is needed.const debouncedFormSave = useCallback( debounce(async (payload: Partial<TSticky>) => { await handleChange(payload); }, 500), - [stickyOperations, stickyData, handleChange] + [handleChange] );packages/ui/src/icons/sticky-note-icon.tsx (1)
26-28
: Fix SVG attribute naming.The SVG attributes use kebab-case instead of camelCase, which is the React convention for SVG attributes.
- fill-rule="evenodd" - clip-rule="evenodd" + fillRule="evenodd" + clipRule="evenodd"web/core/components/stickies/modal/stickies.tsx (1)
63-65
: Optimize scroll performance.The overflow container might cause performance issues with a large number of sticky notes. Consider implementing virtualization for better performance.
Consider using a virtualized list library like
react-window
orreact-virtualized
for theStickiesLayout
component when dealing with many sticky notes.packages/ui/src/icons/multiple-sticky.tsx (1)
5-28
: LGTM! Well-structured SVG icon component.The implementation follows React best practices and maintains consistency with other icon components. The use of
currentColor
ensures proper theme compatibility.Consider adding an
aria-label
for better accessibility:<svg width="20" height="20" viewBox="0 0 20 20" fill="none" xmlns="http://www.w3.org/2000/svg" className={className} + aria-label="Recent sticky notes" {...rest} >
web/core/components/stickies/modal/search.tsx (2)
52-60
: Enhance accessibility for the search input.The search input lacks proper ARIA attributes and role definitions.
<input ref={inputRef} className="w-full max-w-[234px] border-none bg-transparent text-sm text-custom-text-100 placeholder:text-custom-text-400 focus:outline-none" placeholder="Search by title" + role="searchbox" + aria-label="Search sticky notes" value={searchQuery} onChange={(e) => updateSearchQuery(e.target.value)} onKeyDown={handleInputKeyDown} />
23-28
: Improve keyboard event handling.Consider handling more keyboard events for better accessibility and user experience.
const handleInputKeyDown = (e: React.KeyboardEvent<HTMLInputElement>) => { + if (e.key === "Enter") { + e.preventDefault(); // Prevent form submission if within a form + } if (e.key === "Escape") { if (searchQuery && searchQuery.trim() !== "") updateSearchQuery(""); else setIsSearchOpen(false); } };apiserver/plane/app/views/workspace/preference.py (1)
10-10
: Remove unused import.The
Count
import fromdjango.db.models
is not used in this file.-from django.db.models import Count
🧰 Tools
🪛 Ruff (0.8.2)
10-10:
django.db.models.Count
imported but unusedRemove unused import:
django.db.models.Count
(F401)
web/core/components/stickies/sticky/use-operations.tsx (1)
44-51
: Improve error type safety.The
error: any
type should be more specific for better type safety and error handling.- } catch (error: any) { + } catch (error) { + const errorMessage = error instanceof Error ? error.message : + error?.data?.error ?? "The sticky could not be created"; setToast({ - message: error?.data?.error ?? "The sticky could not be created", + message: errorMessage, type: TOAST_TYPE.ERROR, title: "Sticky not created", }); throw error; }web/core/components/home/widgets/recents/index.tsx (1)
67-79
: Consider improving empty state UX.The empty state handling could be enhanced:
- The condition
!loader && joinedProjectIds?.length === 0
might flicker between states.- The min-height constraint in an empty state might create unnecessary whitespace.
Consider this improvement:
- if (!loader && joinedProjectIds?.length === 0) return <EmptyWorkspace />; + if (!loader && joinedProjectIds?.length === 0) { + return ( + <div className="h-full flex items-center justify-center"> + <EmptyWorkspace /> + </div> + ); + }web/core/components/stickies/sticky/inputs.tsx (2)
11-18
: Consider adding validation constraints to props.The props type definition could benefit from additional validation constraints:
stickyData
could have a more specific undefined case handlingworkspaceSlug
should be non-emptyConsider using zod for runtime validation:
import { z } from 'zod'; const PropsSchema = z.object({ stickyData: z.object({/*...*/}).optional(), workspaceSlug: z.string().min(1), // ... other props });
46-60
: Optimize form submission handler.The
handleFormSubmit
includesworkspaceSlug
in its dependencies array but doesn't use it.Remove the unused dependency:
const handleFormSubmit = useCallback( async (formdata: Partial<TSticky>) => { if (formdata.name !== undefined) { await handleUpdate({ description_html: formdata.description_html ?? "<p></p>", name: formdata.name, }); } else { await handleUpdate({ description_html: formdata.description_html ?? "<p></p>", }); } }, - [handleUpdate, workspaceSlug] + [handleUpdate] );web/core/components/editor/sticky-editor/editor.tsx (1)
17-33
: Consider making required props explicit.The props interface extends ILiteTextEditor but omits some props. Consider making the component's requirements more explicit.
Consider this pattern:
interface RequiredProps { workspaceSlug: string; workspaceId: string; uploadFile: (file: File) => Promise<string>; handleColorChange: (data: Partial<TSticky>) => Promise<void>; handleDelete: () => Promise<void>; } interface OptionalProps { projectId?: string; // ... other optional props } interface StickyEditorWrapperProps extends RequiredProps, Partial<OptionalProps>, Omit<ILiteTextEditor, "disabledExtensions" | "fileHandler" | "mentionHandler">web/core/components/editor/sticky-editor/toolbar.tsx (1)
89-92
: Empty object in className conditional.The
cn
utility is used with an empty object, which serves no purpose.Remove the empty object:
className={cn( - "grid place-items-center aspect-square rounded-sm p-0.5 text-custom-text-300", - {} + "grid place-items-center aspect-square rounded-sm p-0.5 text-custom-text-300" )}web/core/components/stickies/action-bar.tsx (1)
26-29
: Optimize data fetching with SWRThe SWR configuration lacks proper caching and revalidation strategies. Consider adding revalidation options to optimize performance.
Apply this diff to improve data fetching:
useSWR( workspaceSlug ? `WORKSPACE_STICKIES_${workspaceSlug}_${PER_PAGE}:${currentPage}:0` : null, workspaceSlug ? () => fetchWorkspaceStickies(workspaceSlug.toString(), `${PER_PAGE}:${currentPage}:0`, PER_PAGE) : null, + { + revalidateOnFocus: false, + revalidateIfStale: false, + dedupingInterval: 10000 + } );Also applies to: 49-54
web/core/components/stickies/stickies-layout.tsx (1)
63-98
: Optimize observer usage and cleanupThe component uses both ResizeObserver and MutationObserver, which might impact performance. Consider combining their functionality or using a more efficient approach.
Consider using a debounced height update function:
+import { debounce } from "lodash"; + useEffect(() => { if (!masonryRef?.current) return; - const updateHeight = () => { + const updateHeight = debounce(() => { if (masonryRef.current) { const height = masonryRef.current.getBoundingClientRect().height; setContainerHeight(parseInt(height.toString())); } - }; + }, 100);web/core/store/base-command-palette.store.ts (1)
250-256
: Simplify modal toggle logicThe toggle implementation can be simplified to match the pattern used in other toggle methods.
Apply this diff to maintain consistency:
toggleAllStickiesModal = (value?: boolean) => { - if (value) { + if (value !== undefined) { this.allStickiesModal = value; } else { this.allStickiesModal = !this.allStickiesModal; } };web/core/constants/editor.ts (1)
Line range hint
1-190
: Consider splitting editor configurations into separate filesThe file currently handles multiple concerns:
- Type definitions
- Toolbar configurations
- PDF styling
Consider splitting these into separate files for better maintainability:
types.ts
for editor typestoolbar-config.ts
for toolbar configurationspdf-styles.ts
for PDF-related stylesThis separation would:
- Make the codebase more maintainable
- Make it easier to test individual concerns
- Reduce the risk of merge conflicts
- Make it clearer where to add new editor-specific configurations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
apiserver/plane/app/views/workspace/preference.py
(2 hunks)packages/types/src/index.d.ts
(1 hunks)packages/types/src/stickies.d copy.ts
(1 hunks)packages/types/src/stickies.d.ts
(1 hunks)packages/ui/src/icons/index.ts
(1 hunks)packages/ui/src/icons/multiple-sticky.tsx
(1 hunks)packages/ui/src/icons/sticky-note-icon.tsx
(1 hunks)web/ce/components/stickies/index.ts
(0 hunks)web/ce/components/stickies/widget.tsx
(0 hunks)web/core/components/editor/index.ts
(1 hunks)web/core/components/editor/sticky-editor/color-pallete.tsx
(1 hunks)web/core/components/editor/sticky-editor/editor.tsx
(1 hunks)web/core/components/editor/sticky-editor/index.ts
(1 hunks)web/core/components/editor/sticky-editor/toolbar.tsx
(1 hunks)web/core/components/home/home-dashboard-widgets.tsx
(2 hunks)web/core/components/home/root.tsx
(2 hunks)web/core/components/home/widgets/empty-states/issues.tsx
(1 hunks)web/core/components/home/widgets/links/root.tsx
(1 hunks)web/core/components/home/widgets/manage/index.tsx
(1 hunks)web/core/components/home/widgets/recents/index.tsx
(3 hunks)web/core/components/stickies/action-bar.tsx
(1 hunks)web/core/components/stickies/empty.tsx
(1 hunks)web/core/components/stickies/index.ts
(1 hunks)web/core/components/stickies/modal/index.tsx
(1 hunks)web/core/components/stickies/modal/search.tsx
(1 hunks)web/core/components/stickies/modal/stickies.tsx
(1 hunks)web/core/components/stickies/stickies-layout.tsx
(1 hunks)web/core/components/stickies/sticky/index.ts
(1 hunks)web/core/components/stickies/sticky/inputs.tsx
(1 hunks)web/core/components/stickies/sticky/root.tsx
(1 hunks)web/core/components/stickies/sticky/use-operations.tsx
(1 hunks)web/core/components/stickies/widget.tsx
(1 hunks)web/core/constants/editor.ts
(2 hunks)web/core/hooks/use-stickies.tsx
(1 hunks)web/core/services/sticky.service.ts
(1 hunks)web/core/store/base-command-palette.store.ts
(7 hunks)web/core/store/root.store.ts
(4 hunks)web/core/store/sticky/sticky.store.ts
(1 hunks)
💤 Files with no reviewable changes (2)
- web/ce/components/stickies/widget.tsx
- web/ce/components/stickies/index.ts
✅ Files skipped from review due to trivial changes (4)
- web/core/components/stickies/sticky/index.ts
- web/core/components/editor/sticky-editor/index.ts
- web/core/components/stickies/index.ts
- web/core/components/home/widgets/links/root.tsx
🧰 Additional context used
🪛 Ruff (0.8.2)
apiserver/plane/app/views/workspace/preference.py
10-10: django.db.models.Count
imported but unused
Remove unused import: django.db.models.Count
(F401)
🪛 Biome (1.9.4)
web/core/store/sticky/sticky.store.ts
[error] 73-73: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-admin
- GitHub Check: build-space
- GitHub Check: lint-web
🔇 Additional comments (9)
web/core/components/editor/index.ts (1)
5-5
: Export statement confirmedThe addition of
export * from "./sticky-editor";
correctly exports the newsticky-editor
module, ensuring it's accessible throughout the application.web/core/components/home/widgets/manage/index.tsx (1)
7-7
: Verify the impact of removing save/cancel buttons.The removal of save/cancel buttons raises concerns about how users will persist their widget management changes.
Please clarify:
- How are widget changes saved now?
- Is the save functionality handled automatically?
- Should we maintain an undo capability for accidental changes?
Run this script to check for alternative save mechanisms:
Also applies to: 20-28
✅ Verification successful
Removal of save/cancel buttons is intentionally replaced with better UX
The widget management now follows a more intuitive direct-manipulation pattern:
- Changes are saved automatically through reactive state management
- Users receive immediate feedback via toast notifications
- Widgets can be toggled on/off directly
- Drag-and-drop reordering with instant persistence
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for widget-related state management rg -g '!*.{css,json,md}' -A 5 'widget.*store|saveWidget|updateWidget' web/Length of output: 76
Script:
#!/bin/bash # Search for widget-related code with broader patterns rg -g '!*.{css,json,md}' -l "widget" web/core/ # Find widget component structure ast-grep --pattern 'const ManageWidgetsModal = observer(($_) => { $$$ })' # Search for modal state management rg -g '!*.{css,json,md}' -A 5 'setLayout|updateLayout|onLayoutChange' web/Length of output: 3313
Script:
#!/bin/bash # Check store implementation rg -g '!*.{css,json,md}' -A 10 'class.*Store' web/core/store/workspace/home.ts rg -g '!*.{css,json,md}' -A 10 'class.*Store' web/core/store/dashboard.store.ts # Check widget helpers cat web/core/components/home/widgets/manage/widget.helpers.ts # Check modal implementation cat web/core/components/home/widgets/manage/index.tsxLength of output: 4239
Script:
#!/bin/bash # Check WidgetList implementation cat web/core/components/home/widgets/manage/widget-list.tsx # Look for widget state updates rg -g '!*.{css,json,md}' -A 5 'setWidgets|updateWidgets|onWidgetChange' web/core/Length of output: 2183
packages/types/src/index.d.ts (1)
42-42
: LGTM! Export addition follows existing pattern.The addition of stickies types export is consistent with the file's structure.
packages/ui/src/icons/index.ts (1)
50-51
: LGTM! Icon exports follow existing pattern.The addition of sticky-related icon exports is consistent with the file's structure.
web/core/components/home/root.tsx (1)
59-70
: Verify the impact of unconditional rendering.The component now renders
IssuePeekOverview
andDashboardWidgets
unconditionally. While this simplifies the code, we should ensure that these components handle their own empty states appropriately.✅ Verification successful
Empty states are properly handled in both components
Both components have robust empty state handling:
- IssuePeekOverview includes dedicated error and loading state components
- DashboardWidgets is connected to a store with proper data management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if IssuePeekOverview and DashboardWidgets handle empty states ast-grep --pattern 'const IssuePeekOverview = $_$ => { $$$ return ( $$$ ) }' ast-grep --pattern 'const DashboardWidgets = $_$ => { $$$ return ( $$$ ) }'Length of output: 176
Script:
#!/bin/bash # Find files containing IssuePeekOverview and DashboardWidgets echo "=== Files containing IssuePeekOverview ===" rg "IssuePeekOverview" --type tsx --type ts echo -e "\n=== Files containing DashboardWidgets ===" rg "DashboardWidgets" --type tsx --type ts echo -e "\n=== Looking for empty state handling patterns ===" rg -i "empty|no.*data|!.*length|===\s*0|<=\s*0" --type tsx --type tsLength of output: 550
Script:
#!/bin/bash # Find files containing IssuePeekOverview and DashboardWidgets echo "=== Files containing IssuePeekOverview ===" rg "IssuePeekOverview" -g "*.tsx" -g "*.ts" echo -e "\n=== Files containing DashboardWidgets ===" rg "DashboardWidgets" -g "*.tsx" -g "*.ts" # Find the actual component definitions echo -e "\n=== Component Definitions ===" rg "export.*(IssuePeekOverview|DashboardWidgets)" -g "*.tsx" -g "*.ts" -A 5 # Look for empty state handling echo -e "\n=== Empty State Handling ===" rg "(!|\.|===|<=)\s*(data|items|issues|widgets)(\.|\.length)?\s*(===|<=|!)?\s*0" -g "*.tsx" -g "*.ts" rg "(no|empty)\s*(data|items|issues|widgets)" -g "*.tsx" -g "*.ts"Length of output: 17129
apiserver/plane/app/views/workspace/preference.py (1)
42-57
: Potential race condition in bulk preference creation.The
sort_order
calculation and bulk creation could lead to race conditions if multiple users access this endpoint simultaneously.Consider using a database transaction to ensure atomicity:
+from django.db import transaction + +with transaction.atomic(): sort_order = 1000 - sort_order_counter preference = WorkspaceHomePreference.objects.bulk_create( [ WorkspaceHomePreference( key=key, user=request.user, workspace=workspace, sort_order=sort_order, ) for key in create_preference_keys ], batch_size=10, ignore_conflicts=True, )web/core/components/home/widgets/recents/index.tsx (1)
11-11
: LGTM! Import statements are well-organized.The addition of
useProject
hook andIssuesEmptyState
component aligns with the changes in empty state handling.Also applies to: 14-14
web/core/store/root.store.ts (1)
63-63
: Consider cleanup in resetOnSignOutThe stickyStore is properly initialized, but ensure that any subscriptions or side effects in the StickyStore are properly cleaned up during sign out.
Run this script to check for potential cleanup methods in the StickyStore:
Also applies to: 92-92, 124-124
web/core/constants/editor.ts (1)
33-33
: Verify toolbar items configuration for the sticky editor typeThe "sticky" type has been added to
TEditorTypes
, but the base toolbar item arrays (BASIC_MARK_ITEMS
,LIST_ITEMS
, etc.) don't include "sticky" in theireditors
arrays. This might cause issues with filtering in the future if we need to checkitem.editors.includes("sticky")
.Run this script to check for potential issues:
createSticky = async (workspaceSlug: string, sticky: Partial<TSticky>) => { | ||
if (!this.showAddNewSticky) return; | ||
this.showAddNewSticky = false; | ||
this.creatingSticky = true; | ||
const workspaceStickies = this.workspaceStickies[workspaceSlug] || []; | ||
const response = await this.stickyService.createSticky(workspaceSlug, sticky); | ||
runInAction(() => { | ||
this.stickies[response.id] = response; | ||
this.workspaceStickies[workspaceSlug] = [response.id, ...workspaceStickies]; | ||
this.activeStickyId = response.id; | ||
this.recentStickyId = response.id; | ||
this.creatingSticky = false; | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add error handling in createSticky
method
Currently, if stickyService.createSticky
fails, the state creatingSticky
remains true
, and no error is logged or handled. It's important to handle potential errors to ensure the application remains stable and provides feedback to the user.
Apply this diff to add error handling:
createSticky = async (workspaceSlug: string, sticky: Partial<TSticky>) => {
if (!this.showAddNewSticky) return;
this.showAddNewSticky = false;
this.creatingSticky = true;
+ try {
const workspaceStickies = this.workspaceStickies[workspaceSlug] || [];
const response = await this.stickyService.createSticky(workspaceSlug, sticky);
runInAction(() => {
this.stickies[response.id] = response;
this.workspaceStickies[workspaceSlug] = [response.id, ...workspaceStickies];
this.activeStickyId = response.id;
this.recentStickyId = response.id;
this.creatingSticky = false;
});
+ } catch (e) {
+ console.error(e);
+ runInAction(() => {
+ this.creatingSticky = 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.
createSticky = async (workspaceSlug: string, sticky: Partial<TSticky>) => { | |
if (!this.showAddNewSticky) return; | |
this.showAddNewSticky = false; | |
this.creatingSticky = true; | |
const workspaceStickies = this.workspaceStickies[workspaceSlug] || []; | |
const response = await this.stickyService.createSticky(workspaceSlug, sticky); | |
runInAction(() => { | |
this.stickies[response.id] = response; | |
this.workspaceStickies[workspaceSlug] = [response.id, ...workspaceStickies]; | |
this.activeStickyId = response.id; | |
this.recentStickyId = response.id; | |
this.creatingSticky = false; | |
}); | |
}; | |
createSticky = async (workspaceSlug: string, sticky: Partial<TSticky>) => { | |
if (!this.showAddNewSticky) return; | |
this.showAddNewSticky = false; | |
this.creatingSticky = true; | |
try { | |
const workspaceStickies = this.workspaceStickies[workspaceSlug] || []; | |
const response = await this.stickyService.createSticky(workspaceSlug, sticky); | |
runInAction(() => { | |
this.stickies[response.id] = response; | |
this.workspaceStickies[workspaceSlug] = [response.id, ...workspaceStickies]; | |
this.activeStickyId = response.id; | |
this.recentStickyId = response.id; | |
this.creatingSticky = false; | |
}); | |
} catch (e) { | |
console.error(e); | |
runInAction(() => { | |
this.creatingSticky = false; | |
}); | |
} | |
}; |
deleteSticky = async (workspaceSlug: string, id: string) => { | ||
const sticky = this.stickies[id]; | ||
if (!sticky) return; | ||
try { | ||
this.workspaceStickies[workspaceSlug] = this.workspaceStickies[workspaceSlug].filter( | ||
(stickyId) => stickyId !== id | ||
); | ||
if (this.activeStickyId === id) this.activeStickyId = undefined; | ||
delete this.stickies[id]; | ||
this.recentStickyId = this.workspaceStickies[workspaceSlug][0]; | ||
await this.stickyService.deleteSticky(workspaceSlug, id); | ||
} catch (e) { | ||
console.log(e); | ||
this.stickies[id] = sticky; | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle potential errors in deleteSticky
method
Similar to createSticky
, if stickyService.deleteSticky
fails, the sticky is already removed from the state. This could lead to inconsistencies between the client and the server.
Consider rearranging the code to remove the sticky from the state only after a successful deletion. Here's a suggested change:
deleteSticky = async (workspaceSlug: string, id: string) => {
const sticky = this.stickies[id];
if (!sticky) return;
try {
+ await this.stickyService.deleteSticky(workspaceSlug, id);
this.workspaceStickies[workspaceSlug] = this.workspaceStickies[workspaceSlug].filter(
(stickyId) => stickyId !== id
);
if (this.activeStickyId === id) this.activeStickyId = undefined;
delete this.stickies[id];
this.recentStickyId = this.workspaceStickies[workspaceSlug][0];
- await this.stickyService.deleteSticky(workspaceSlug, id);
} catch (e) {
console.log(e);
+ // Optionally, handle error by informing the user
}
};
📝 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.
deleteSticky = async (workspaceSlug: string, id: string) => { | |
const sticky = this.stickies[id]; | |
if (!sticky) return; | |
try { | |
this.workspaceStickies[workspaceSlug] = this.workspaceStickies[workspaceSlug].filter( | |
(stickyId) => stickyId !== id | |
); | |
if (this.activeStickyId === id) this.activeStickyId = undefined; | |
delete this.stickies[id]; | |
this.recentStickyId = this.workspaceStickies[workspaceSlug][0]; | |
await this.stickyService.deleteSticky(workspaceSlug, id); | |
} catch (e) { | |
console.log(e); | |
this.stickies[id] = sticky; | |
} | |
}; | |
deleteSticky = async (workspaceSlug: string, id: string) => { | |
const sticky = this.stickies[id]; | |
if (!sticky) return; | |
try { | |
await this.stickyService.deleteSticky(workspaceSlug, id); | |
this.workspaceStickies[workspaceSlug] = this.workspaceStickies[workspaceSlug].filter( | |
(stickyId) => stickyId !== id | |
); | |
if (this.activeStickyId === id) this.activeStickyId = undefined; | |
delete this.stickies[id]; | |
this.recentStickyId = this.workspaceStickies[workspaceSlug][0]; | |
} catch (e) { | |
console.log(e); | |
// Optionally, handle error by informing the user | |
} | |
}; |
export type TSticky = { | ||
id: string; | ||
name?: string; | ||
description_html?: string; | ||
color?: string; | ||
createdAt?: Date; | ||
updatedAt?: Date; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unintended duplicate file detected
The file name stickies.d copy.ts
suggests this is a duplicate or temporary copy. Including duplicate type definitions can cause confusion and potential conflicts.
Recommend removing this file if it's unnecessary or renaming it appropriately if it serves a unique purpose.
export type TSticky = { | ||
id: string; | ||
name?: string; | ||
description_html?: string; | ||
color?: string; | ||
createdAt?: Date; | ||
updatedAt?: Date; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider adding essential fields for data ownership and UI placement.
The TSticky
type might benefit from additional fields:
- Workspace/user association fields for data ownership (e.g.,
workspace_id
,created_by
) - Position/order fields for UI placement (e.g.,
position
,order
) - Color validation (e.g., hex format)
Consider extending the type:
export type TSticky = {
id: string;
name?: string;
description_html?: string;
color?: string;
+ workspace_id: string;
+ created_by: string;
+ position?: { x: number; y: number };
+ order?: number;
createdAt?: Date;
updatedAt?: Date;
};
📝 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 type TSticky = { | |
id: string; | |
name?: string; | |
description_html?: string; | |
color?: string; | |
createdAt?: Date; | |
updatedAt?: Date; | |
}; | |
export type TSticky = { | |
id: string; | |
name?: string; | |
description_html?: string; | |
color?: string; | |
workspace_id: string; | |
created_by: string; | |
position?: { x: number; y: number }; | |
order?: number; | |
createdAt?: Date; | |
updatedAt?: Date; | |
}; |
{STICKY_COLORS.map((color, index) => ( | ||
<button | ||
key={index} | ||
type="button" | ||
onClick={() => handleUpdate({ color })} | ||
className="h-6 w-6 rounded-md hover:ring-2 hover:ring-custom-primary focus:outline-none focus:ring-2 focus:ring-custom-primary transition-all" | ||
style={{ backgroundColor: color }} | ||
/> | ||
))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve button accessibility and React key usage.
The buttons need aria-labels for accessibility, and using index as key might cause issues with React reconciliation.
- {STICKY_COLORS.map((color, index) => (
+ {Object.entries(STICKY_COLORS).map(([name, color]) => (
<button
- key={index}
+ key={color}
type="button"
onClick={() => handleUpdate({ color })}
+ aria-label={`Set sticky note color to ${name.toLowerCase()}`}
className="h-6 w-6 rounded-md hover:ring-2 hover:ring-custom-primary focus:outline-none focus:ring-2 focus:ring-custom-primary transition-all"
style={{ backgroundColor: color }}
/>
))}
Committable suggestion skipped: line range outside the PR's diff.
// TODO: update this while toolbar homogenization | ||
// @ts-expect-error type mismatch here | ||
editorRef?.executeMenuItemCommand({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO comment and type assertion need attention.
There's a TODO comment about toolbar homogenization and a type assertion that needs to be addressed.
This could lead to runtime errors. Consider:
- Creating proper types for the toolbar commands
- Adding runtime checks for the command structure
interface EditorCommand {
itemKey: string;
extraProps?: Record<string, unknown>;
}
function isValidCommand(command: unknown): command is EditorCommand {
return typeof command === 'object' && command !== null && 'itemKey' in command;
}
useEffect(() => { | ||
if (!editorRef) return; | ||
const unsubscribe = editorRef.onStateChange(updateActiveStates); | ||
updateActiveStates(); | ||
return () => unsubscribe(); | ||
}, [editorRef, updateActiveStates]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Potential memory leak in useEffect.
The effect properly sets up and cleans up the state change subscription, but there's a potential race condition if editorRef
changes rapidly.
Add a cleanup flag:
useEffect(() => {
if (!editorRef) return;
+ let mounted = true;
const unsubscribe = editorRef.onStateChange(updateActiveStates);
- updateActiveStates();
+ if (mounted) updateActiveStates();
- return () => unsubscribe();
+ return () => {
+ mounted = false;
+ unsubscribe();
+ };
}, [editorRef, updateActiveStates]);
📝 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.
useEffect(() => { | |
if (!editorRef) return; | |
const unsubscribe = editorRef.onStateChange(updateActiveStates); | |
updateActiveStates(); | |
return () => unsubscribe(); | |
}, [editorRef, updateActiveStates]); | |
useEffect(() => { | |
if (!editorRef) return; | |
let mounted = true; | |
const unsubscribe = editorRef.onStateChange(updateActiveStates); | |
if (mounted) updateActiveStates(); | |
return () => { | |
mounted = false; | |
unsubscribe(); | |
}; | |
}, [editorRef, updateActiveStates]); |
<Tooltip tooltipContent="All stickies" isMobile={false} position="left"> | ||
<button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add aria-labels to improve accessibility
The buttons lack proper aria-labels, which makes them inaccessible to screen readers. This affects the "Add sticky" button and other action buttons.
Apply this diff to improve accessibility:
<button
+ aria-label="Add sticky note"
className="btn btn--icon rounded-full w-10 h-10 flex items-center justify-center shadow-sm bg-custom-background-100"
onClick={() => {
updateActiveStickyId("");
toggleShowNewSticky(true);
setNewSticky(true);
}}
>
Also applies to: 84-95
{/* @ts-expect-error type mismatch here */} | ||
<Masonry elementType="div">{childElements}</Masonry> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix TypeScript type mismatch with Masonry component
The code suppresses a TypeScript error with @ts-expect-error. This should be properly typed.
Consider using proper types for the Masonry component:
-{/* @ts-expect-error type mismatch here */}
-<Masonry elementType="div">{childElements}</Masonry>
+<Masonry
+ elementType="div"
+ options={{
+ transitionDuration: 0,
+ initLayout: true
+ }}
+>
+ {childElements}
+</Masonry>
Committable suggestion skipped: line range outside the PR's diff.
sticky: { | ||
basic: BASIC_MARK_ITEMS.filter((item) => ["Bold", "Italic"].includes(item.name)), | ||
list: LIST_ITEMS.filter((item) => ["To-do list"].includes(item.name)), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve sticky toolbar configuration robustness and consistency
The current implementation has several potential issues:
- Uses fragile name-based filtering instead of editor type-based filtering
- String literals aren't type-safe
- Inconsistent with other editor configurations that have more sections
Consider this more robust approach:
sticky: {
- basic: BASIC_MARK_ITEMS.filter((item) => ["Bold", "Italic"].includes(item.name)),
- list: LIST_ITEMS.filter((item) => ["To-do list"].includes(item.name)),
+ basic: BASIC_MARK_ITEMS.filter((item) => {
+ // Add "sticky" to the editors array where needed
+ return ["bold", "italic"].includes(item.itemKey);
+ }),
+ list: LIST_ITEMS.filter((item) => {
+ // Add "sticky" to the editors array where needed
+ return ["to-do-list"].includes(item.itemKey);
+ }),
},
This approach:
- Uses type-safe
itemKey
instead of string names - Follows the pattern used by other editor types
- Makes it easier to maintain and extend
Committable suggestion skipped: line range outside the PR's diff.
@@ -0,0 +1,8 @@ | |||
export type TSticky = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate file
import { ISvgIcons } from "./type"; | ||
|
||
export const RecentStickyIcon: React.FC<ISvgIcons> = ({ className = "text-current", ...rest }) => ( | ||
<svg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are icons, any reason to add the icons as component rather than import as assets of type SVG?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wanted to control the color, that's why making it component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That can be also made while using svg as an asset and change color while using it.
})} | ||
> | ||
<div className="h-full w-full" ref={masonryRef}> | ||
{/* @ts-expect-error type mismatch here */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see this error a lot in this PR, is it possible to recheck the errors and try to fix the type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its only for the masonry component cause this package doesn't handle typescritp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, i remember seeing 3-4 places in this PR, can you please confirm if none of them can be fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (2)
web/core/components/stickies/sticky/inputs.tsx (2)
36-43
:⚠️ Potential issuePotential race condition in form reset.
The form reset effect doesn't handle the case where
stickyData
is undefined butstickyId
exists.useEffect(() => { if (!stickyId) return; + if (!stickyData) return; reset({ id: stickyId, description_html: stickyData?.description_html === "" ? "<p></p>" : stickyData?.description_html, name: stickyData?.name, }); }, [stickyData, reset]);
45-59
:⚠️ Potential issueAdd error handling for form submissions.
Both the title and description changes trigger immediate form submissions without error handling.
async (formdata: Partial<TSticky>) => { + try { if (formdata.name !== undefined) { await handleUpdate({ description_html: formdata.description_html ?? "<p></p>", name: formdata.name, }); } else { await handleUpdate({ description_html: formdata.description_html ?? "<p></p>", }); } + } catch (error) { + console.error("Failed to update sticky:", error); + // TODO: Add user notification + } }Remove unused dependency.
workspaceSlug
is listed in the dependency array but isn't used in the callback.- [handleUpdate, workspaceSlug] + [handleUpdate]
🧹 Nitpick comments (2)
web/core/components/stickies/sticky/inputs.tsx (2)
10-17
: Add prop validation for workspaceSlug.Consider adding runtime validation to ensure
workspaceSlug
is not empty, as it's crucial for workspace operations.type TProps = { - workspaceSlug: string; + workspaceSlug: string & {}; // ensures non-empty string at compile time
68-78
: Add accessibility attributes to TextArea.The TextArea component lacks proper accessibility attributes.
<TextArea value={value} id="name" name="name" + aria-label="Sticky note title" onChange={(e) => { onChange(e.target.value); handleSubmit(handleFormSubmit)(); }} placeholder="Title" className="text-lg font-medium text-[#455068] mb-2 w-full p-0 border-none min-h-[22px]" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/core/components/stickies/sticky/inputs.tsx
(1 hunks)web/ee/components/stickies/index.ts
(0 hunks)
💤 Files with no reviewable changes (1)
- web/ee/components/stickies/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint-admin
- GitHub Check: lint-space
- GitHub Check: lint-web
- GitHub Check: Analyze (javascript)
// computed values | ||
const workspaceId = getWorkspaceBySlug(workspaceSlug)?.id as string; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add error handling for missing workspace.
The workspace ID computation assumes getWorkspaceBySlug
always returns a value. Add error handling for cases where the workspace isn't found.
- const workspaceId = getWorkspaceBySlug(workspaceSlug)?.id as string;
+ const workspace = getWorkspaceBySlug(workspaceSlug);
+ if (!workspace?.id) {
+ throw new Error(`Workspace not found for slug: ${workspaceSlug}`);
+ }
+ const workspaceId = workspace.id;
📝 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.
// computed values | |
const workspaceId = getWorkspaceBySlug(workspaceSlug)?.id as string; | |
// computed values | |
const workspace = getWorkspaceBySlug(workspaceSlug); | |
if (!workspace?.id) { | |
throw new Error(`Workspace not found for slug: ${workspaceSlug}`); | |
} | |
const workspaceId = workspace.id; | |
}} | ||
placeholder={"Click to type here"} | ||
containerClassName={"px-0 text-base min-h-[200px] w-full text-[#455068]"} | ||
uploadFile={async () => ""} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Implement proper file upload handling using available services
The current implementation is incomplete. Other components in the codebase demonstrate the correct pattern:
- Use
fileService.uploadProjectAsset
or appropriate upload service - Handle the file parameter
- Return the asset ID after successful upload
🔗 Analysis chain
Implement proper file upload handling.
The uploadFile
prop currently returns an empty string without actual implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for file upload implementations in other components
rg -A 5 "uploadFile.*=.*async.*=>" --type typescript
Length of output: 89
Script:
#!/bin/bash
# Search for uploadFile implementations and props
rg -A 5 "uploadFile.*=.*async"
# Search for the component's props interface
ast-grep --pattern 'interface $_ {
$$$
uploadFile
$$$
}'
# Look for file upload related utilities
rg -l "upload.*file|file.*upload" --ignore-case
Length of output: 8454
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (5)
web/core/components/home/widgets/recents/page.tsx (1)
33-39
: Remove unnecessary Fragment.The Fragment wrapper is redundant here as it contains only one child and is not keyed.
- <> {pageDetails?.logo_props?.in_use ? ( <Logo logo={pageDetails?.logo_props} size={16} type="lucide" /> ) : ( <FileText className="h-4 w-4 text-custom-text-300" /> )} - </>🧰 Tools
🪛 Biome (1.9.4)
[error] 33-39: 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)
web/core/components/home/widgets/recents/project.tsx (1)
28-28
: Fix gap inconsistency.The gap-4 spacing in the logo container differs from the gap-2 used in other recent components, which could affect visual consistency.
- <div className="flex flex-shrink-0 items-center justify-center rounded gap-4 bg-custom-background-80 w-[25.5px] h-[25.5px]"> + <div className="flex flex-shrink-0 items-center justify-center rounded gap-2 bg-custom-background-80 w-[25.5px] h-[25.5px]">web/core/components/stickies/modal/stickies.tsx (3)
6-6
: Consider restructuring import paths for better maintainability.The relative import path
../../editor/sticky-editor/color-pallete.tsx
is quite deep. Consider moving shared constants likeSTICKY_COLORS
to a dedicated constants directory or using path aliases for better maintainability.
63-65
: Consider improving the scrollable content area.The current implementation has several potential issues:
- Use
overflow-y-auto
instead ofoverflow-scroll
to avoid showing unnecessary scroll bars- Add
scrollbar-gutter: stable
to prevent layout shifts- Consider implementing virtualization for better performance with many sticky notes
-<div className="mb-4 max-h-[625px] overflow-scroll"> +<div className="mb-4 max-h-[625px] overflow-y-auto scrollbar-gutter-stable">
15-68
: Consider adding error boundaries for better error handling.The component could benefit from error boundaries to gracefully handle runtime errors in the sticky notes functionality. This would prevent the entire UI from breaking if there are issues with sticky operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
web/core/components/home/widgets/recents/index.tsx
(3 hunks)web/core/components/home/widgets/recents/issue.tsx
(1 hunks)web/core/components/home/widgets/recents/page.tsx
(1 hunks)web/core/components/home/widgets/recents/project.tsx
(1 hunks)web/core/components/stickies/modal/stickies.tsx
(1 hunks)web/core/components/stickies/widget.tsx
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/core/components/home/widgets/recents/index.tsx
- web/core/components/stickies/widget.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
web/core/components/home/widgets/recents/page.tsx
[error] 33-39: 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)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: lint-apiserver
- GitHub Check: lint-web
- GitHub Check: lint-space
- GitHub Check: lint-admin
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
web/core/components/home/widgets/recents/page.tsx (1)
31-43
: LGTM! Consistent layout improvements.The layout adjustments (25.5px dimensions and reduced gap spacing) align well with the changes in other recent item components, creating a more cohesive visual experience.
🧰 Tools
🪛 Biome (1.9.4)
[error] 33-39: 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)
web/core/components/home/widgets/recents/issue.tsx (1)
31-31
: LGTM! Consistent sizing update.The addition of size="lg" to IssueIdentifier aligns with the visual consistency improvements across recent item components.
web/core/components/stickies/modal/stickies.tsx (2)
31-51
: Enhance accessibility for the sticky creation button.The button's loading state could be better communicated to screen readers.
17-19
: Add error handling for missing workspace context.The workspace slug could be undefined, but there's no error handling or fallback UI. Consider adding validation and error states.
Summary
This PR adds stickies to home page
Summary by CodeRabbit
Release Notes: Sticky Notes Feature
New Features
StickyActionBar
,StickiesWidget
,AllStickiesModal
,StickyEditor
,Toolbar
, andColorPalette
Improvements
User Experience
Technical Enhancements