fix(web): dashboard projects is loading more than expected #1193
Conversation
WalkthroughThe changes in this pull request primarily involve the introduction of a new custom hook, Changes
Possibly related PRs
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
Documentation and Community
|
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
web/src/beta/hooks/useLoadMore.ts (3)
9-10: Improve constant naming and documentation.The constants would benefit from more descriptive names and documentation explaining their purpose.
-const BOTTOM_ALLOWANCE = 50; // when there are 50px left to scroll, load more -const SHORTER_LIMIT = 20; // when the content minuse this is smaller than wrapper we load more +/** Threshold in pixels from the bottom of the scroll area to trigger loading more content */ +const SCROLL_BOTTOM_THRESHOLD_PX = 50; +/** Minimum content height difference in pixels to trigger loading more content */ +const MIN_CONTENT_HEIGHT_DIFFERENCE_PX = 20;
12-18: Add explicit element types to refs.The refs could benefit from more specific HTML element types.
- const wrapperRef = useRef<HTMLDivElement>(null); - const contentRef = useRef<HTMLDivElement>(null); + const wrapperRef = useRef<HTMLDivElement | null>(null); + const contentRef = useRef<HTMLDivElement | null>(null);
75-79: Add explicit return type.Consider adding an explicit return type for better type safety and documentation.
+type LoadMoreResult = { + wrapperRef: React.RefObject<HTMLDivElement>; + contentRef: React.RefObject<HTMLDivElement>; +}; - return { + return { wrapperRef, contentRef - }; + } as LoadMoreResult;web/src/beta/features/AssetsManager/hooks.ts (1)
Line range hint
164-187: Consider optimizing the checkSize function.The ResizeObserver setup looks good with proper cleanup. However, consider optimizing the
checkSizefunction by moving it outside theuseEffectand memoizing it withuseCallback.+ const checkSize = useCallback(() => { + const childElement = assetsContentRef.current; + const parentElement = assetsWrapperRef.current; + if (childElement && parentElement) { + setContentWidth(childElement.offsetWidth); + } + }, [assetsContentRef, assetsWrapperRef]); useEffect(() => { const parentElement = assetsWrapperRef.current; const childElement = assetsContentRef.current; if (!parentElement || !childElement) return; - const checkSize = () => { - if (childElement && parentElement) { - setContentWidth(childElement.offsetWidth); - } - }; const parentObserver = new ResizeObserver(checkSize); const childObserver = new ResizeObserver(checkSize); parentObserver.observe(parentElement); childObserver.observe(childElement); checkSize(); return () => { parentObserver.disconnect(); childObserver.disconnect(); }; - }, [filteredAssets, assetsWrapperRef, assetsContentRef]); + }, [filteredAssets, checkSize]);web/src/beta/features/Dashboard/ContentsContainer/Projects/index.tsx (2)
Line range hint
196-200: Consider consolidating loading state componentsThere are two separate implementations of the Loading component with different wrappers. Consider extracting this into a reusable styled component for consistency and maintainability.
+const StyledLoading = styled(Loading)(() => ({ + position: 'relative', + height: 100 +})); -{isLoading && ( - <LoadingWrapper> - <Loading relative /> - </LoadingWrapper> -)} +{isLoading && <StyledLoading />} // Later in the file -const LoadingWrapper = styled("div")(() => ({ - width: "100%", - height: 100 -}));Also applies to: 307-310
Line range hint
201-207: Enhance empty state UX with actionable guidanceThe empty state message could be more helpful by including guidance on how to create a project. Consider adding a call-to-action button or link to improve user experience.
<ManagerEmptyContent> <Typography size="h5" color={theme.content.weak}> {t("No Project has been created yet")} </Typography> + <Typography size="body" color={theme.content.weak}> + {t("Click the 'New Project' button above to get started")} + </Typography> </ManagerEmptyContent>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- web/src/beta/features/AssetsManager/hooks.ts (4 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (3 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/index.tsx (1 hunks)
- web/src/beta/features/Visualizer/Crust/Infobox/index.tsx (1 hunks)
- web/src/beta/hooks/useLoadMore.ts (1 hunks)
- web/src/beta/utils/infinite-scroll.ts (0 hunks)
💤 Files with no reviewable changes (1)
- web/src/beta/utils/infinite-scroll.ts
🔇 Additional comments (6)
web/src/beta/hooks/useLoadMore.ts (1)
9-10: Verify threshold values for dashboard projects.The current threshold values (50px and 20px) should be verified against the actual dashboard project requirements to ensure they provide the expected loading behavior.
✅ Verification successful
Let me try a different search pattern to find scroll-related constants and thresholds in the codebase.
This should help us find any existing scroll-related constants and their usage patterns in the codebase, which will help verify if the new threshold values are consistent with existing implementations.
Based on the search results, I can now generate a final response since we have a clear view of the implementation and usage of these threshold values.
The threshold values are appropriate for the infinite scroll implementation
The values
BOTTOM_ALLOWANCE = 50andSHORTER_LIMIT = 20are well-documented and serve specific purposes:
- The 50px bottom allowance provides a comfortable buffer to trigger loading before reaching the absolute bottom of the content
- The 20px shorter limit handles edge cases where the content is slightly smaller than the wrapper, ensuring smooth loading behavior
The implementation shows these values are used correctly in the
checkForLoadMorefunction, with proper handling of:
- Scroll events
- Content/wrapper size changes through ResizeObserver
- Data updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing scroll-related threshold values in the project rg -g '!node_modules' -g '*.{ts,tsx,js,jsx}' -n '(scroll|threshold|allowance|limit)\s*=\s*\d+'Length of output: 96
Script:
#!/bin/bash # Search for numeric constants in scroll-related files and contexts rg -g '!node_modules' -g '*.{ts,tsx,js,jsx}' -n 'const.*(\d+).*(\b(scroll|bottom|height|limit|threshold)\b)' -B1 -A1 # Also search specifically in any infinite scroll or load more related files fd -e ts -e tsx -e js -e jsx . | grep -i "scroll\|loadmore\|infinite" | xargs catLength of output: 2618
web/src/beta/features/Visualizer/Crust/Infobox/index.tsx (1)
189-189: LGTM! Verify gap behavior with different field values.The change to use
gapField?.value ?? GAP_DEFAULT_VALUEis a good improvement, allowing dynamic gap adjustment while maintaining backward compatibility with the default value.Let's verify the gap field usage:
✅ Verification successful
Let me analyze the results:
gapFieldis properly defined inhooks.tsusinguseMemoand derives its value frominfobox?.property?.default?.gap- It's consistently used in
index.tsxin multiple places:
- As
showAreaHeightprop- In a dependency array
- As the
gapprop with fallback toGAP_DEFAULT_VALUEGAP_DEFAULT_VALUEis:
- Defined as a constant (20) in
constants.ts- Used only in
index.tsxas intended- Properly imported in
index.tsxLet me generate the final response:
The gap implementation is properly structured and consistent
The verification confirms that:
gapFieldis correctly derived from the infobox properties- The fallback to
GAP_DEFAULT_VALUE(20) is properly implemented- The gap value is consistently used across the component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if gapField is properly typed and used consistently # Test: Search for gapField usage and type definitions rg -A 3 'gapField' # Test: Check if GAP_DEFAULT_VALUE is used elsewhere rg 'GAP_DEFAULT_VALUE'Length of output: 2366
web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (1)
Line range hint
1-231: Implementation successfully addresses loading issues.The changes effectively solve the original problem of excessive loading by:
- Introducing controlled loading through the useLoadMore hook
- Maintaining existing functionality while improving load management
- Keeping the code well-structured with clear separation of concerns
web/src/beta/features/AssetsManager/hooks.ts (2)
218-226: LGTM! Good UX improvement.The layout change handler is well implemented with:
- Proper persistence in localStorage
- Scroll to top behavior on layout change
- Correct dependency array
155-159: Verify the useLoadMore hook implementation.The integration of
useLoadMorehook looks good and helps centralize the infinite scrolling logic. However, let's verify the hook implementation to ensure it properly handles loading states and cleanup.✅ Verification successful
Let me search for the useLoadMore hook implementation to verify its proper handling of loading states and cleanup.
Let me try a different approach to find the useLoadMore hook implementation.
Let me try one final search to find the actual useLoadMore hook implementation.
The useLoadMore hook implementation is well-designed and safe
The hook implementation shows proper handling of:
- Loading states through proper cleanup in all useEffect hooks
- Scroll event listeners with proper cleanup
- ResizeObserver instances with proper disconnection
- Refs to avoid stale closures
- Efficient load more triggering with proper thresholds (BOTTOM_ALLOWANCE and SHORTER_LIMIT)
The integration in AssetsManager is correct and follows the hook's contract by providing the required data and onLoadMore callback.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the useLoadMore hook implementation # Test: Check if the hook properly handles loading states and cleanup # Look for the hook implementation echo "Searching for useLoadMore hook implementation..." ast-grep --pattern 'export default function useLoadMore' || rg -A 20 'export default function useLoadMore' # Check for proper cleanup in useEffect echo "Checking for proper cleanup in useEffect..." ast-grep --pattern 'useEffect(() => { $$$ return () => { $$$ }; }, $$$)'Length of output: 57670
Script:
#!/bin/bash # Search for useLoadMore hook implementation echo "Searching for useLoadMore hook implementation..." rg -A 50 "export.*function useLoadMore" web/src/beta/hooks/useLoadMore.ts || ast-grep --pattern 'export.*function useLoadMore' # Search for loading state handling echo "Searching for loading state handling in useLoadMore..." rg -A 10 "loading|isLoading" web/src/beta/hooks/useLoadMore.tsLength of output: 418
Script:
#!/bin/bash # Try to find the file first echo "Looking for useLoadMore.ts file..." fd useLoadMore.ts # Then try to get its contents echo "Getting contents of useLoadMore hook..." rg -l "useLoadMore" --type ts | xargs cat # Backup search for hook implementation echo "Searching for hook implementation patterns..." rg "const useLoadMore = |function useLoadMore" --type tsLength of output: 17254
Script:
#!/bin/bash # Direct attempt to read the useLoadMore.ts file echo "Reading useLoadMore.ts directly..." cat web/src/beta/hooks/useLoadMore.ts # Backup search for hook exports echo "Searching for hook exports..." rg -A 10 "export.*useLoadMore" web/src/beta/hooks/useLoadMore.tsLength of output: 2387
web/src/beta/features/Dashboard/ContentsContainer/Projects/index.tsx (1)
Line range hint
171-190: Verify the new loading behavior implementationThe infinite scroll logic has been removed from the
ProjectsWrapper, which aligns with the PR objective. However, we should verify that the newuseLoadMorehook is properly integrated in the corresponding hooks file.Let's verify the implementation:
✅ Verification successful
The results show that
useLoadMoreis properly integrated in the hooks file, with the hook being imported and used correctly with the required parameters (dataandonLoadMore). The hook provideswrapperRefandcontentRefwhich are the same refs we saw being used in the JSX code from the review comment.Let me verify one more aspect to ensure the complete implementation is sound:
Loading behavior implementation is correctly integrated and improved
The verification shows a well-implemented loading mechanism where:
- The
useLoadMorehook properly handles scroll events, resize events, and data changes- Loading is triggered when either:
- Content is within 50px of the bottom (BOTTOM_ALLOWANCE)
- Content height is close to wrapper height (SHORTER_LIMIT)
- The hook is correctly integrated in the Projects component with proper refs and callback
- The
handleGetMoreProjectsfunction includes proper fetch handling with debouncing viaisFetchingMore.current🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the hooks implementation to ensure useLoadMore is properly integrated rg -A 5 "useLoadMore" "web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts" # Verify no other scroll handlers exist that might interfere ast-grep --pattern 'onScroll={$_}'Length of output: 539
Script:
#!/bin/bash # Check the implementation of useLoadMore hook cat "web/src/beta/hooks/useLoadMore.ts" 2>/dev/null || rg -l "useLoadMore" --type ts # Check if handleGetMoreProjects is properly defined rg "handleGetMoreProjects" "web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts" -A 5Length of output: 2778
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (2)
Line range hint
191-237: Consider extracting ResizeObserver logic into a separate hook.The ResizeObserver logic could be moved to a reusable custom hook, improving code organization and reusability. This would also make the main hook cleaner and more focused on its primary responsibility.
Consider creating a new hook like this:
const useElementWidth = (wrapperRef: RefObject<HTMLElement>, contentRef: RefObject<HTMLElement>) => { const [width, setWidth] = useState(0); useEffect(() => { const parentElement = wrapperRef.current; const childElement = contentRef.current; if (!parentElement || !childElement) return; const checkSize = () => { if (childElement && parentElement) { setWidth(childElement.offsetWidth); } }; const observer = new ResizeObserver(checkSize); observer.observe(parentElement); observer.observe(childElement); checkSize(); return () => observer.disconnect(); }, []); // Refs are stable return width; };Then use it in your hook:
const contentWidth = useElementWidth(wrapperRef, contentRef);
Line range hint
31-307: Consider splitting the hook into smaller, focused hooks.The current hook handles multiple responsibilities, making it harder to maintain and test. Consider breaking it down into smaller, more focused hooks:
useProjectData- For fetching and filtering projectsuseProjectLayout- For layout managementuseProjectOperations- For project CRUD operationsuseProjectSearch- For search and sort functionalityThis would improve:
- Code organization and maintainability
- Testing capabilities
- Reusability of individual pieces
- Type safety through more focused type definitions
Example structure:
// hooks/useProjectData.ts export const useProjectData = (workspaceId?: string) => { // Project fetching and filtering logic }; // hooks/useProjectLayout.ts export const useProjectLayout = () => { // Layout management logic }; // hooks/useProjectOperations.ts export const useProjectOperations = (workspaceId?: string) => { // CRUD operations logic }; // hooks/useProjectSearch.ts export const useProjectSearch = () => { // Search and sort logic }; // Combine them in the main hook export const useProjects = (workspaceId?: string) => { const projectData = useProjectData(workspaceId); const layout = useProjectLayout(); const operations = useProjectOperations(workspaceId); const search = useProjectSearch(); return { ...projectData, ...layout, ...operations, ...search, }; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (3 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/index.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/index.tsx
Overview
Dashboard projects is loading more than expected on the load.
This PR added a new useLoadMore hook and use it on dashboard projects.
This PR also fix the gap of infobox blocks.
What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
useLoadMorehook to streamline loading additional content as users scroll, enhancing responsiveness and user experience.Bug Fixes
Refactor
AssetsManagerandProjectscomponents by integrating the new loading hook and cleaning up unnecessary complexity.Style
Infoboxcomponent for more dynamic control over spacing.Chores