[WEB-3092] fix: fixed recents + removed home route #6365
Conversation
WalkthroughThe pull request involves significant changes to the web application's frontend structure, primarily focusing on removing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
web/core/components/core/content-overflow-HOC.tsx (3)
75-87: Add cleanup for transition event listener when showAll changesThe transition event listener cleanup might miss some cases since it only runs on unmount. Consider adding
showAllto the effect dependencies to properly cleanup and reattach the listener when the state changes.- useEffect(() => { + useEffect(() => { if (!containerRef.current) return; const handleTransitionEnd = () => { setIsTransitioning(false); }; containerRef.current.addEventListener("transitionend", handleTransitionEnd); return () => { containerRef.current?.removeEventListener("transitionend", handleTransitionEnd); }; - }, []); + }, [showAll]);
135-140: Enhance button accessibilityThe show more/less button should have proper ARIA attributes for better screen reader support.
<button className={cn( "gap-1 w-full text-custom-primary-100 text-sm font-medium transition-opacity duration-300", buttonClassName )} onClick={handleToggle} disabled={isTransitioning} + aria-expanded={showAll} + aria-controls={contentRef.current?.id} + aria-label={showAll ? "Show less content" : "Show more content"} >
110-117: Add unique ID for ARIA controls referenceThe content div needs a unique ID to be referenced by the button's aria-controls attribute.
<div ref={contentRef} + id={`content-${Math.random().toString(36).substr(2, 9)}`} className={cn("h-auto", { "pb-6": showAll, })} > {children} </div>web/core/components/home/widgets/recents/index.tsx (1)
98-100: Add error boundary or fallback for invalid activitiesWhile filtering out activities without entity_data is good, consider adding error handling for malformed activity data to prevent rendering failures.
recents - .filter((recent: TActivityEntityData) => recent.entity_data) + .filter((recent: TActivityEntityData) => { + try { + return recent && recent.entity_data && recent.id; + } catch (error) { + console.error("Invalid activity data:", error); + return false; + } + })web/core/components/home/widgets/links/links.tsx (2)
37-37: Remove unnecessary empty fragmentThe empty fragment as fallback can be replaced with null.
- fallback={<></>} + fallback={null}🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
(lint/complexity/noUselessFragments)
Line range hint
83-102: Great architectural decision with ContentOverflowWrapperThe implementation of ContentOverflowWrapper as a reusable component for handling content overflow is a solid architectural decision. It provides:
- Consistent overflow behavior across components
- Smooth transitions with proper state management
- Dynamic content handling with ResizeObserver
- Performance optimization by preventing multiple clicks during transitions
Consider documenting these capabilities in the component's JSDoc for better maintainability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
(lint/complexity/noUselessFragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
web/app/[workspaceSlug]/(projects)/home/header.tsx(0 hunks)web/app/[workspaceSlug]/(projects)/home/page.tsx(0 hunks)web/core/components/core/content-overflow-HOC.tsx(2 hunks)web/core/components/home/widgets/links/links.tsx(1 hunks)web/core/components/home/widgets/recents/index.tsx(3 hunks)
💤 Files with no reviewable changes (2)
- web/app/[workspaceSlug]/(projects)/home/header.tsx
- web/app/[workspaceSlug]/(projects)/home/page.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
web/core/components/home/widgets/links/links.tsx
[error] 37-37: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
(lint/complexity/noUselessFragments)
web/core/components/home/widgets/recents/index.tsx
[error] 86-86: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
…to fix-home-improvements
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apiserver/plane/app/views/workspace/recent_visit.py (2)
25-32: Consider optimizing the entity filtering logicThe current implementation uses two separate filters for entity_name. Consider:
- Combining the filters for better query efficiency
- Moving the entity types to a constant/enum for better maintainability
Here's a suggested improvement:
+VALID_ENTITY_TYPES = ["issue", "page", "project"] class UserRecentVisitViewSet(BaseViewSet): # ... existing code ... def list(self, request, slug): user_recent_visits = UserRecentVisit.objects.filter( workspace__slug=slug, user=request.user ) entity_name = request.query_params.get("entity_name") - if entity_name: - user_recent_visits = user_recent_visits.filter(entity_name=entity_name) - - user_recent_visits = user_recent_visits.filter( - entity_name__in=["issue", "page", "project"] - ) + user_recent_visits = user_recent_visits.filter( + entity_name__in=[entity_name] if entity_name in VALID_ENTITY_TYPES else VALID_ENTITY_TYPES + )
34-35: Optimize query performance with database-level limitingInstead of fetching all records and slicing in memory, consider using
.order_by()and[:20]at the query level for better performance with large datasets.- serializer = WorkspaceRecentVisitSerializer(user_recent_visits[:20], many=True) + user_recent_visits = user_recent_visits.order_by('-created_at')[:20] + serializer = WorkspaceRecentVisitSerializer(user_recent_visits, many=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apiserver/plane/app/views/workspace/recent_visit.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
apiserver/plane/app/views/workspace/recent_visit.py (3)
Line range hint
12-20: LGTM! Well-structured ViewSet with appropriate permissionsThe ViewSet is properly configured with:
- Correct inheritance from BaseViewSet
- Appropriate serializer class selection
- Proper workspace-level permission checks for all user roles
21-23: Great security improvement with user-specific filtering!The addition of
user=request.userensures that users can only access their own recent visits within the workspace context.
21-35: Verify the impact on the recent activities widgetThe changes properly fix the recent visits functionality by:
- Ensuring proper user scoping
- Filtering valid entity types
- Limiting results appropriately
However, since this is part of the "fixed recents" PR objective, please verify:
✅ Verification successful
Recent visits API changes are compatible with frontend implementation
The backend changes align perfectly with the frontend service implementation in
web/core/services/workspace.service.ts. ThefetchWorkspaceRecentsmethod correctly handles the optionalentity_nameparameter and the API endpoint matches the backend implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any related frontend changes that might depend on these backend changes rg -l "UserRecentVisit|recent.*visit" web/Length of output: 83
Script:
#!/bin/bash # Check the implementation details in the frontend service cat web/core/services/workspace.service.tsLength of output: 11455
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
Release Notes
Refactor
ContentOverflowWrapperfor improved layout and overflow handling.Bug Fixes
Performance