-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[WEB-5542] chore: scrollIntoView implementation for sidebar active project #8208
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
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds smooth scrolling for the active project in the workspace sidebar: when the URL project ID matches the item, the project list opens and the active item is scrolled into view smoothly and centered after a 200ms delay. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
apps/web/core/components/workspace/sidebar/projects-list-item.tsx
Outdated
Show resolved
Hide resolved
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: 0
🧹 Nitpick comments (1)
apps/web/core/components/workspace/sidebar/projects-list-item.tsx (1)
228-244: Clean upsetTimeoutto prevent stale execution on unmount.The
setTimeoutis not cleared if the component unmounts or the effect re-runs within the 100ms window. While the innerprojectRef.currentcheck prevents a crash, it's still good practice to clean up timers.Apply this diff to add proper cleanup:
useEffect(() => { + let timeoutId: ReturnType<typeof setTimeout> | undefined; if (URLProjectId === project?.id) { setIsProjectListOpen(true); // Scroll to active project if (projectRef.current) { - setTimeout(async () => { + timeoutId = setTimeout(() => { if (projectRef.current) { - await scrollIntoView(projectRef.current, { + scrollIntoView(projectRef.current, { behavior: "smooth", block: "center", scrollMode: "if-needed", }); } }, 100); } } + return () => { + if (timeoutId) clearTimeout(timeoutId); + }; }, [URLProjectId, project?.id, setIsProjectListOpen]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/core/components/workspace/sidebar/projects-list-item.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
apps/web/core/components/workspace/sidebar/projects-list-item.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-21T17:22:05.204Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7989
File: apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx:45-46
Timestamp: 2025-10-21T17:22:05.204Z
Learning: In the makeplane/plane repository, the refactor from useParams() to params prop is specifically scoped to page.tsx and layout.tsx files in apps/web/app (Next.js App Router pattern). Other components (hooks, regular client components, utilities) should continue using the useParams() hook as that is the correct pattern for non-route components.
Applied to files:
apps/web/core/components/workspace/sidebar/projects-list-item.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Agent
- GitHub Check: Build and lint web apps
🔇 Additional comments (1)
apps/web/core/components/workspace/sidebar/projects-list-item.tsx (1)
10-10: LGTM!The import is correct for the
smooth-scroll-into-view-if-neededlibrary.
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.
Pull request overview
This PR implements automatic scrolling to the active project in the sidebar when a project is selected. The implementation uses the smooth-scroll-into-view-if-needed library to smoothly scroll the active project into view with a center alignment when it matches the URL project ID.
Key changes:
- Added
smooth-scroll-into-view-if-neededimport for conditional smooth scrolling - Wrapped the scroll logic in a 100ms delayed timeout to allow DOM updates to complete before scrolling
- Applied scroll with "if-needed" mode to avoid unnecessary scrolling when the element is already visible
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/web/core/components/workspace/sidebar/projects-list-item.tsx
Outdated
Show resolved
Hide resolved
apps/web/core/components/workspace/sidebar/projects-list-item.tsx
Outdated
Show resolved
Hide resolved
apps/web/core/components/workspace/sidebar/projects-list-item.tsx
Outdated
Show resolved
Hide resolved
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
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: 0
🧹 Nitpick comments (1)
apps/web/core/components/workspace/sidebar/projects-list-item.tsx (1)
228-252: Active-project scroll effect looks correct; optional constant for delayThe effect cleanly:
- Opens the project list when the URL project matches.
- Schedules a scroll with a 200 ms delay and re-checks the ref before scrolling.
- Clears the timeout on cleanup to avoid leaks.
Logic and dependencies look good and address earlier feedback (no unnecessary
async/await, proper cleanup). If this delay value is reused elsewhere, consider extracting200into a shared constant to avoid magic numbers, but that’s not required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/core/components/workspace/sidebar/projects-list-item.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
apps/web/core/components/workspace/sidebar/projects-list-item.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-21T17:22:05.204Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7989
File: apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx:45-46
Timestamp: 2025-10-21T17:22:05.204Z
Learning: In the makeplane/plane repository, the refactor from useParams() to params prop is specifically scoped to page.tsx and layout.tsx files in apps/web/app (Next.js App Router pattern). Other components (hooks, regular client components, utilities) should continue using the useParams() hook as that is the correct pattern for non-route components.
Applied to files:
apps/web/core/components/workspace/sidebar/projects-list-item.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apps/web/core/components/workspace/sidebar/projects-list-item.tsx (1)
10-10: New scroll helper import is appropriateImporting
scrollIntoViewhere is straightforward and its usage is correctly confined to a client-side effect; no changes needed.
Description
This PR includes the implementation of scrollIntoView for the active project in the sidebar.
Type of Change
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.