[WEB-4108]chore: refactored project wrapper #7066
Conversation
WalkthroughA new data synchronization system for project resources was introduced using custom React hooks. The logic for fetching and synchronizing project data was consolidated from multiple SWR hooks into a single core hook, Changes
Sequence Diagram(s)sequenceDiagram
participant ProjectAuthWrapper
participant useProjectResources
participant useProjectCoreResources
participant SWR
participant GlobalStore
ProjectAuthWrapper->>useProjectResources: Call with workspaceSlug, projectId
useProjectResources->>useProjectCoreResources: Call with workspaceSlug, projectId
useProjectCoreResources->>SWR: Fetch project data (details, members, labels, etc.)
SWR-->>useProjectCoreResources: Return data
useProjectCoreResources->>GlobalStore: Sync data via side effects
useProjectResources-->>ProjectAuthWrapper: Return { isLoading: false }
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (2)
web/ce/hooks/use-project-resources.ts (2)
34-39: Consider adding a dependency to the useEffectThe ESLint disable comment suggests that there might be missing dependencies in the useEffect dependency array.
useEffect(() => { initGantt(); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [initGantt]);Alternatively, if
initGanttshould only run once regardless of changes to the function reference, consider moving it outside the component or memoizing it.
56-121: Consider abstracting the repeated SWR patternThere's a repeating pattern across multiple SWR calls with similar conditional logic. This could potentially be abstracted into a helper function.
+ // Helper function to reduce repetition + const fetchProjectResource = ( + key: string, + fetcher: () => Promise<any>, + options: any = { revalidateIfStale: false, revalidateOnFocus: false } + ) => { + return useSWR( + workspaceSlug && projectId ? `${key}_${workspaceSlug}_${projectId}` : null, + workspaceSlug && projectId ? () => fetcher() : null, + options + ); + }; + // Project labels -useSWR( - workspaceSlug && projectId ? `PROJECT_LABELS_${workspaceSlug}_${projectId}` : null, - workspaceSlug && projectId ? () => fetchProjectLabels(workspaceSlug.toString(), projectId.toString()) : null, - { revalidateIfStale: false, revalidateOnFocus: false } -); +fetchProjectResource( + "PROJECT_LABELS", + () => fetchProjectLabels(workspaceSlug!.toString(), projectId!.toString()) +);This would make the code more DRY and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
web/app/[workspaceSlug]/(projects)/browse/[workItem]/page.tsx(3 hunks)web/app/[workspaceSlug]/(projects)/projects/(detail)/layout.tsx(1 hunks)web/ce/hooks/use-project-resources.ts(1 hunks)web/ce/layouts/project-wrapper.tsx(0 hunks)web/core/layouts/auth-layout/index.ts(0 hunks)web/core/layouts/auth-layout/project-wrapper.tsx(0 hunks)web/ee/layouts/project-wrapper.tsx(0 hunks)
💤 Files with no reviewable changes (4)
- web/core/layouts/auth-layout/index.ts
- web/core/layouts/auth-layout/project-wrapper.tsx
- web/ee/layouts/project-wrapper.tsx
- web/ce/layouts/project-wrapper.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
web/app/[workspaceSlug]/(projects)/browse/[workItem]/page.tsx (3)
web/core/store/router.store.ts (3)
projectId(85-87)issueId(149-151)workspaceSlug(69-71)web/ce/hooks/use-project-resources.ts (1)
useProjectResources(124-126)web/core/components/issues/issue-detail/root.tsx (1)
IssueDetailRoot(62-392)
web/app/[workspaceSlug]/(projects)/projects/(detail)/layout.tsx (1)
web/ce/hooks/use-project-resources.ts (1)
useProjectResources(124-126)
web/ce/hooks/use-project-resources.ts (3)
web/core/store/router.store.ts (2)
workspaceSlug(69-71)projectId(85-87)web/core/hooks/store/use-cycle.ts (1)
useCycle(7-11)web/core/local-db/storage.sqlite.ts (1)
persistence(475-475)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (11)
web/app/[workspaceSlug]/(projects)/browse/[workItem]/page.tsx (4)
19-19: Great job replacing the wrapper component with a hook!The import change from
ProjectAuthWrappertouseProjectResourcesaligns with modern React practices of using hooks for shared logic instead of wrapper components.
49-49: Good use of nullish coalescing operator for fallback valueUsing
??to provide a default empty string ensuresprojectIdis always a string, which prevents type-related issues downstream.
56-57: Nice explanatory comment for the hook usageAdding the comment "Load project resources when needed" clearly explains the purpose of the hook call, which is good for maintainability.
102-109: Clean removal of the wrapper componentThe code is now more straightforward by directly rendering
IssueDetailRootwithout the extra wrapper layer, which simplifies the component tree.web/app/[workspaceSlug]/(projects)/projects/(detail)/layout.tsx (3)
5-6: Good organization with the "hooks" commentAdding the comment to categorize imports improves code readability and organization.
11-14: Clear comment and well-placed hook callThe explanatory comment and hook call are placed at the top level of the component, ensuring resources are loaded early.
15-15: Simplified return statementDirectly returning children without the wrapper component simplifies the component tree and reduces unnecessary nesting.
web/ce/hooks/use-project-resources.ts (4)
1-19: Well-organized importsThe imports are nicely organized with comments separating different categories, which improves readability.
20-33: Good consolidation of resource hooksThe hook effectively consolidates all the necessary resource hooks in one place, making it easier to manage project-related data fetching.
40-54: Smart use of SWR for issue synchronizationUsing SWR with conditional fetching based on workspace and project ID availability ensures that synchronization only happens when needed. The refresh interval of 5 minutes is also a reasonable choice.
123-126: Good wrapper hook for API consistencyCreating a dedicated
useProjectResourceswrapper hook provides a clean API for components and allows for future extensibility.
vamsikrishnamathala
changed the title
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
vamsikrishnamathala
force-pushed
the
chore-project_wrapper_refactor
branch
from
bb27156 to
eb14414
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
web/core/hooks/use-project-resources.ts (3)
34-38: Consider adding cleanup logic for the Gantt chart.The hook initializes the Gantt chart on mount but doesn't provide cleanup logic. To prevent potential memory leaks, consider adding a cleanup function in the useEffect return.
useEffect(() => { initGantt(); // eslint-disable-next-line react-hooks/exhaustive-deps + return () => { + // Add cleanup logic here if needed + }; }, []);
68-94: Consider abstracting repeated SWR pattern.This block shows multiple SWR calls with the same revalidation settings. Consider creating a utility function to reduce repetition.
// A helper function to reduce repetition const fetchProjectResource = ( key: string, fetcher: () => Promise<any>, options = { revalidateIfStale: false, revalidateOnFocus: false } ) => { return useSWR( workspaceSlug && projectId ? `${key}_${workspaceSlug}_${projectId}` : null, workspaceSlug && projectId ? () => fetcher() : null, options ); }; // Example usage fetchProjectResource( "PROJECT_LABELS", () => fetchProjectLabels(workspaceSlug!.toString(), projectId!.toString()) );
115-121: Consider exposing loading and error states.The hook currently triggers side effects but doesn't provide consumers with loading or error states. Consider returning these states to improve usability.
// Example of how the hook could return loading and error states export const useProjectCoreResources = (workspaceSlug?: string, projectId?: string) => { // ... existing code ... // Track overall loading and error states const [isLoading, setIsLoading] = useState(true); const [error, setError] = useState<Error | null>(null); // Use useEffect to track when all resources are loaded useEffect(() => { // Logic to determine when all resources are loaded // and update isLoading and error states }, [/* dependencies */]); return { isLoading, error }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
web/app/[workspaceSlug]/(projects)/browse/[workItem]/page.tsx(3 hunks)web/ce/hooks/use-project-resources.ts(1 hunks)web/core/hooks/use-project-resources.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/ce/hooks/use-project-resources.ts
- web/app/[workspaceSlug]/(projects)/browse/[workItem]/page.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/core/hooks/use-project-resources.ts (3)
web/core/store/router.store.ts (2)
workspaceSlug(69-71)projectId(85-87)web/core/hooks/store/use-cycle.ts (1)
useCycle(7-11)web/core/local-db/storage.sqlite.ts (1)
persistence(475-475)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
web/core/hooks/use-project-resources.ts (6)
1-19: Clean and well-organized imports.The imports are clearly organized into React core, third-party libraries, and internal hooks. This makes the file easy to understand and maintain.
20-33: Good hook design with clear dependencies.The hook has a well-defined signature with optional parameters and properly initializes all required dependencies from various store hooks. This design allows for flexible usage across different components.
40-54: Well-implemented issue synchronization with refresh interval.The implementation uses SWR effectively with appropriate cache keys and revalidation settings. The 5-minute refresh interval ensures data stays reasonably up-to-date while not overwhelming the server with requests.
56-66: Proper fetching of project details and user info.The implementation correctly fetches project details and user project information conditionally based on the availability of workspace and project IDs.
96-101: Consistent fetch pattern for project cycles.This implementation correctly follows the established pattern for fetching project resources with appropriate cache keys and revalidation settings.
20-121: Great refactoring from component to hook.This refactoring successfully moves from a component-based approach to a hook-based approach, improving separation of concerns. The hook focuses solely on data fetching and synchronization, making it more reusable across different UI components.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
web/ce/hooks/use-project-resources.ts (1)
4-9: 🛠️ Refactor suggestionPropagate the real loading state instead of returning a hard-coded value
useProjectCoreResourcesalready returns anisLoadingflag but the wrapper discards it and always returnsfalse.
IfuseProjectCoreResourcesis later extended to do any async work (very likely), components relying onuseProjectResourceswill never know that data is still being fetched and may render incomplete UI.Proposed minimal fix:
export const useProjectResources = (workspaceSlug?: string, projectId?: string) => { - useProjectCoreResources(workspaceSlug, projectId); - return { - isLoading: false, - }; + const { isLoading } = useProjectCoreResources(workspaceSlug, projectId); + return { isLoading }; };This keeps the wrapper thin while preserving useful state for consumers.
🧹 Nitpick comments (1)
web/core/layouts/auth-layout/project-wrapper.tsx (1)
64-64: Leverage theisLoadingflag returned by the new hookThe call to
useProjectResourcesis made, but its return value is ignored.
CapturingisLoadingwould let the wrapper avoid duplicating the existing “parent loading” logic and prevent potential flashes of empty UI if the underlying hook ever becomes asynchronous (see previous comment).- useProjectResources(workspaceSlug?.toString(), projectId?.toString()); + const { isLoading: isProjectLoading } = useProjectResources( + workspaceSlug?.toString(), + projectId?.toString() + );You can then merge
isProjectLoadingwithisParentLoadingin the early-return spinner block.This keeps the wrapper future-proof and in sync with the hook’s contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
web/ce/hooks/use-project-resources.ts(1 hunks)web/core/hooks/use-project-resources.ts(1 hunks)web/core/layouts/auth-layout/project-wrapper.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/hooks/use-project-resources.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/ce/hooks/use-project-resources.ts (1)
web/core/hooks/use-project-resources.ts (1)
useProjectCoreResources(17-115)
vamsikrishnamathala
force-pushed
the
chore-project_wrapper_refactor
branch
from
fa6c4df to
7a6ff6e
Compare
Description
This update refactors
ProjectAuthWrapper. All the SWR calls are aggregated into a single re-usable hook.Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit