fix: manage widgets integrations #6331
Conversation
…t-home-integrations
WalkthroughThis pull request introduces comprehensive changes to the home dashboard widget system across multiple files. The modifications primarily focus on enhancing the type safety and flexibility of widget management, introducing new widget keys, and refactoring the widget rendering and reordering logic. The changes span type definitions, component implementations, and store management, creating a more robust and extensible widget ecosystem. Changes
Possibly related PRs
Suggested Labels
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 (
|
…/plane into fix-home-header
![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 (7)
web/core/components/home/widgets/manage/widget-list.tsx (3)
6-7: Leverage import compactness or named re-exports.MobX’s observer and your custom setToast are well organized. If other modules also need these, consider a centralized re-export to simplify import statements across files.
30-43: Include error information in catch block.You're showing a generic error toast if something goes wrong. Including the actual error message or logging it may help troubleshooting. For example:
} catch (error) { setToast({ type: TOAST_TYPE.ERROR, title: "Error!", - message: "Error occurred while reordering widget.", + message: error instanceof Error ? error.message : "Error occurred while reordering widget.", }); }
60-60: Consider returning the component for non-React usage cases.If there's a possibility of reusing
WidgetListlogic or data outside of React, you might factor out the reorder/toggle logic into a helper or store method for better modularity.web/core/components/home/widgets/manage/widget.helpers.ts (2)
14-14: Docstring Tautology.
"the dragging widget data that is being dragged" is slightly repetitive. Consider simplifying this string.- * @param source the dragging widget data that is being dragged on the dropTarget + * @param source the widget data currently being dragged onto the dropTarget
40-40: Typographical error in comment.
"cannon" should be "cannot."- // if source that is being dragged is a group. A group cannon be a child of any other widget, + // if source that is being dragged is a group. A group cannot be a child of any other widget,web/core/store/workspace/home.ts (1)
93-100: Debugging logs for reorder flow.
The log statements can be helpful for debugging. Consider removing them once stable.web/core/components/home/widgets/manage/widget-item.tsx (1)
114-114: Reassess the disabled exhaustive-deps rule
You’ve disabledreact-hooks/exhaustive-deps. Confirm that re-running this effect only whenwidget.key,isDragging, andisLastChildchange is intended. If so, document the reason or restructure to avoid disabling this lint rule.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/types/src/home.d.ts(2 hunks)web/core/components/home/home-dashboard-widgets.tsx(2 hunks)web/core/components/home/root.tsx(4 hunks)web/core/components/home/user-greetings.tsx(1 hunks)web/core/components/home/widgets/links/links.tsx(1 hunks)web/core/components/home/widgets/links/root.tsx(1 hunks)web/core/components/home/widgets/loaders/loader.tsx(1 hunks)web/core/components/home/widgets/manage/widget-item.tsx(4 hunks)web/core/components/home/widgets/manage/widget-list.tsx(2 hunks)web/core/components/home/widgets/manage/widget.helpers.ts(3 hunks)web/core/components/home/widgets/recents/filters.tsx(1 hunks)web/core/components/home/widgets/recents/index.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/store/workspace/home.ts(6 hunks)web/ee/components/home/header.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- web/ee/components/home/header.tsx
- web/core/components/home/widgets/recents/filters.tsx
- web/core/components/home/widgets/links/root.tsx
- web/core/components/home/user-greetings.tsx
- web/core/components/home/widgets/recents/project.tsx
- web/core/components/home/widgets/links/links.tsx
- web/core/components/home/widgets/recents/page.tsx
🔇 Additional comments (28)
packages/types/src/home.d.ts (1)
72-72: Great improvement using stricter typing.Changing
key: stringtokey: THomeWidgetKeysenforces compile-time checks and improves type safety.web/core/components/home/home-dashboard-widgets.tsx (2)
28-28: Good use of store fields for better state management.Extracting
toggleWidgetSettings,widgetsMap,showWidgetSettings, andorderedWidgetsfromuseHome()is a clean approach to keep widget state organized. No issues spotted here.
41-45: Conditional rendering looks solid.The logic to only render widgets that have a valid component and are enabled is clear and efficient. Avoid rendering placeholders unless needed.
web/core/components/home/widgets/manage/widget-list.tsx (2)
12-13: Kudos for going fully reactive withobserver.Wrapping
WidgetListwithobserverensures seamless reaction to store updates. This approach eliminates stale UI states.
49-55: Dynamic rendering based on ordered widgets is well structured.Using
orderedWidgets.map(...)keeps the list flexible and sorted. Keys are set properly, and toggling the widget is straightforward. This approach is maintainable and scalable.web/core/components/home/widgets/manage/widget.helpers.ts (3)
2-2: Imports appear consistent and aligned with new widget data types.
No issues with the modified import line.
48-53: Function documentation updated correctly.
The refactoring of param name from “favorite” to “widget” aligns with the new type usage. No functional issues identified.
58-59: Self-dropping condition is handled properly.
This guard prevents dropping a widget onto itself.web/core/components/home/widgets/recents/index.tsx (3)
6-7: SWR and lucide-react imports look good.
No concerns here; usage is consistent with best practices for data fetching and icons.
10-13: Imports reorganization.
The updated imports forLayersIcon,WorkspaceService,EmptyWorkspace, andWidgetLoadermaintain clarity.
17-17: Retained import forRecentProject.
The reference toRecentProjectremains valid and consistent.web/core/components/home/root.tsx (5)
4-4: Use of SWR for data fetching.
Switching to SWR can improve revalidation logic and performance. No issues identified.
14-14: Consolidated hooks import.
No problems with grouping additional hooks under a single import statement.
32-32: New store methods usage.
toggleWidgetSettingsandfetchWidgetsare more semantically accurate than the old method naming.
36-43: SWR config alignment.
Properly disabling revalidation in certain scenarios can reduce unnecessary calls. Looks effective and well-used here.
67-67: Joined project IDs check.
Rendering logic to show empty state or the widgets is clearly structured.web/core/store/workspace/home.ts (6)
3-4: MobX and type imports are correct.
No issues spotted in the updated import statements.
12-14: Enhanced type specificity for widgets.
Changing fromstring[]toTHomeWidgetKeys[]clarifies usage and helps maintain type safety.
28-28: Proper initialization ofwidgets.
Assigning an empty array ofTHomeWidgetKeys[]is consistent with the new type definition.
40-41: Computed property definition is structured correctly.
TheorderedWidgetsproperty is set up appropriately for MobX.
55-57:orderedWidgetscomputation.
Sorting widgets by descendingsort_orderensures the correct display order.
67-67: Sorting before mapping.
Settingthis.widgetsbased on sorted data ensures consistent ordering across the application.web/core/components/home/widgets/manage/widget-item.tsx (6)
17-17: Anticipate undefinedworkspaceSlug
WhileuseParams()typically providesworkspaceSlug, there's a possibility it may beundefined. Consider adding a fallback or guard clause to avoid runtime issues for routes missingworkspaceSlug.
30-33: Props update aligns well with new widget mapping
The newwidgetIdandhandleToggleprops cleanly separate widget identification from toggling logic, improving clarity and manageability.
48-48: Ensurewidgetis not undefined
widgetsMap[widgetId]could be undefined if thewidgetIddoesn’t exist in the store. Consider adding a check or fallback to avoid potential runtime errors.
Line range hint
55-72: Confirmwidget.keystability for drag-and-drop
Usingwidget.keyas the primary identifier in draggable operations is valid ifwidget.keyremains stable across re-renders. Ifwidget.keycan change, or if there are potential conflicts, consider using a more unique or persistent ID.
130-130: Readable display transformation
Replacing underscores with spaces is a quick way to improve readability. This is a simple but effective enhancement for user-facing labels.
132-135: Validate toggle usage
CallinghandleToggle(workspaceSlug.toString(), widget.key, !widget.is_enabled)is straightforward. However, confirm thatworkspaceSlugis always a valid, non-empty string to prevent undesired toggling behavior in edge-case routes.
| import { QuickLinksWidgetLoader } from "./quick-links"; | ||
| import { RecentActivityWidgetLoader } from "./recent-activity"; |
There was a problem hiding this comment.
Potential mismatch between widget keys.
You are importing RecentActivityWidgetLoader and mapping it to EWidgetKeys.RECENT_ACTIVITY = "recent_activity", but in other files, the key "recents" is used. This mismatch may cause rendering or typing issues. Please ensure the widget key is consistently named across all files.
| @@ -2,7 +2,7 @@ import { TLogoProps } from "./common"; | |||
| import { TIssuePriorities } from "./issues"; | |||
|
|
|||
| export type TRecentActivityFilterKeys = "all item" | "issue" | "page" | "project"; | |||
| export type THomeWidgetKeys = "quick_links" | "recent_activity" | "stickies"; | |||
| export type THomeWidgetKeys = "quick_links" | "recents" | "my_stickies" | "quick_tutorial" | "new_at_plane"; | |||
There was a problem hiding this comment.
Ensure consistent naming for the recent activity widget.
Here, you define "recents" as part of THomeWidgetKeys. However, in loader.tsx, the enum uses "recent_activity". Please unify the naming (e.g., use all "recents" or all "recent_activity") to avoid potential type mismatches.
| [key in THomeWidgetKeys]: { component: React.FC<THomeWidgetProps> | null; fullWidth: boolean }; | ||
| } = { | ||
| quick_links: { component: DashboardQuickLinks, fullWidth: false }, | ||
| recent_activity: { component: RecentActivityWidget, fullWidth: false }, | ||
| stickies: { component: StickiesWidget, fullWidth: false }, | ||
| recents: { component: RecentActivityWidget, fullWidth: false }, | ||
| my_stickies: { component: StickiesWidget, fullWidth: false }, | ||
| new_at_plane: { component: null, fullWidth: false }, | ||
| quick_tutorial: { component: null, fullWidth: false }, |
There was a problem hiding this comment.
Confirm unified widget keys usage.
Defining "recents", "my_stickies", etc., is consistent with the updated THomeWidgetKeys. Nonetheless, be sure to align any usage of "recent_activity" in the codebase (e.g., EWidgetKeys.RECENT_ACTIVITY) with "recents" to prevent runtime errors.
gakshita
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
web/core/store/workspace/home.ts (3)
55-57: Consider reducing code duplication.The sorting and mapping logic is duplicated between
orderedWidgetsgetter andfetchWidgetsmethod. Consider reusing the getter to maintain consistency:fetchWidgets = async (workspaceSlug: string) => { try { const widgets = await this.workspaceService.fetchWorkspaceWidgets(workspaceSlug); runInAction(() => { - this.widgets = orderBy(Object.values(widgets), "sort_order", "desc").map((widget) => widget.key); + widgets.forEach((widget) => { + this.widgetsMap[widget.key] = widget; + }); + this.widgets = this.orderedWidgets; - widgets.forEach((widget) => { - this.widgetsMap[widget.key] = widget; - }); }); } catch (error) {Also applies to: 67-67
Line range hint
89-112: Consider improving the widget reordering strategy.The current implementation has potential issues:
- Using fixed increments/decrements of 10000 could lead to sort_order overflow
- Floating-point division might cause precision issues
- No periodic normalization of sort_order values
Consider these improvements:
- Implement periodic rebalancing of sort_order values
- Use integer arithmetic instead of floating-point
- Add bounds checking for sort_order values
- Consider using a more scalable ordering algorithm like Fractal Tree ordering
Would you like me to provide a more detailed implementation of these improvements?
Line range hint
71-74: Enhance error handling with more context.The current error messages could be more informative for debugging purposes.
} catch (error) { - console.error("Failed to fetch widgets"); + console.error("Failed to fetch widgets:", { + error, + workspaceSlug, + context: "HomeStore.fetchWidgets" + }); throw error; }Apply similar improvements to other error handlers in the file.
Also applies to: 83-86, 113-116
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/core/store/workspace/home.ts(5 hunks)
🔇 Additional comments (2)
web/core/store/workspace/home.ts (2)
3-4: LGTM! Type safety improvements.The changes enhance type safety by using the specific
THomeWidgetKeystype instead of generic strings, making the code more maintainable and less error-prone.Also applies to: 12-14
28-28: LGTM! Proper MobX store setup.The store is correctly configured with observable properties and the new computed property is properly registered in the constructor.
Also applies to: 40-41
|
@gakshita Code looks good, Please resolve conflicts |
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates