[WEB-5536] feat: prevent search panels from reopening on programmatic focus restoration #8207
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 a new hook to centralize expandable-search UI state and handlers; refactors the top navigation to use that hook; updates a responsive tab ResizeObserver to depend on the container ref; conditionally renders the right-side action group in the sidebar only when the title is "Projects"; minor formatting changes to two locale files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,mts,cts}📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
Files:
⏰ 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)
🔇 Additional comments (1)
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 |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/core/components/navigation/top-nav-power-k.tsx (1)
131-199: Add missing dependency to handleKeyDown callback.The
handleKeyDowncallback usesisOpendirectly (lines 162, 187) but doesn't include it in the dependency array (line 198). This creates a stale closure where the callback retains the old value ofisOpen, potentially causing the arrow key and Enter key handlers to operate on outdated state.Apply this diff to add the missing dependency:
[searchTerm, activePage, context, shouldShowContextBasedActions, setActivePage, closePanel] + [searchTerm, activePage, context, shouldShowContextBasedActions, setActivePage, closePanel, isOpen] );Note:
containerRefdoesn't need to be added since refs are stable across renders.
🧹 Nitpick comments (2)
apps/web/core/hooks/use-expandable-search.ts (2)
4-6: Export the type for better developer experience.The
UseExpandableSearchOptionstype should be exported so consuming code can reference it for type safety and documentation purposes.Apply this diff to export the type:
-type UseExpandableSearchOptions = { +export type UseExpandableSearchOptions = { onClose?: () => void; };
50-55: Consider stabilizing openPanel to avoid unnecessary re-renders.The
openPanelcallback includesisOpenas a dependency, causing it to be recreated whenever the panel opens or closes. This could trigger unnecessary re-renders in consuming components that depend on this callback.Consider using a functional state update to eliminate the dependency:
// Helper to open panel (for typing/onChange) -const openPanel = useCallback(() => { - if (!isOpen) { - setIsOpen(true); - } -}, [isOpen]); +const openPanel = useCallback(() => { + setIsOpen((prev) => prev ? prev : true); +}, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/core/components/navigation/top-nav-power-k.tsx(9 hunks)apps/web/core/hooks/use-expandable-search.ts(1 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/navigation/top-nav-power-k.tsxapps/web/core/hooks/use-expandable-search.ts
🧠 Learnings (2)
📚 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/navigation/top-nav-power-k.tsx
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.
Applied to files:
apps/web/core/components/navigation/top-nav-power-k.tsx
🧬 Code graph analysis (1)
apps/web/core/components/navigation/top-nav-power-k.tsx (1)
apps/web/core/hooks/use-expandable-search.ts (1)
useExpandableSearch(12-72)
⏰ 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: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (2)
apps/web/core/components/navigation/top-nav-power-k.tsx (2)
34-49: Well-integrated hook usage!The integration of the
useExpandableSearchhook is clean and properly centralizes the expandable search behavior. TheonClosecallback correctly resets the related state, and the destructured values are used appropriately throughout the component.
223-228: Excellent implementation of the user interaction distinction.The combination of
openPanelon input change andhandleMouseDownon the input element correctly distinguishes between user-initiated focus (which should open the panel) and programmatic focus restoration (which should not). This directly addresses the PR objective of preventing unwanted panel reopening.
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where search panels would inadvertently reopen when focus was restored programmatically (rather than by user interaction). The solution distinguishes between user-initiated focus (via mouse clicks) and programmatic focus by tracking mouse-down events. Additionally, the PR refactors the expandable search behavior into a reusable useExpandableSearch hook that can be shared across components.
Key changes:
- Created a new
useExpandableSearchhook that manages focus state and prevents unwanted panel reopening on programmatic focus restoration - Refactored
TopNavPowerKcomponent to use the new hook, reducing code duplication and improving maintainability - Added
onMouseDownhandler to track explicit user clicks, distinguishing them from programmatic focus events
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/web/core/hooks/use-expandable-search.ts | New reusable hook implementing expandable search logic with focus management, outside-click detection, and programmatic vs. user-initiated focus distinction |
| apps/web/core/components/navigation/top-nav-power-k.tsx | Refactored to use the new useExpandableSearch hook, removing duplicate state/ref management and consolidating panel close logic into a single closePanel function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/core/components/navigation/use-responsive-tab-layout.ts (1)
45-45: Optional chaining is unnecessary.The optional chaining operator (
?.) on line 45 is redundant.containerRefis initialized withuseRef<HTMLDivElement>(null)on line 34, so it's nevernullorundefined—only its.currentproperty can benull.Apply this diff:
- const container = containerRef?.current; + const container = containerRef.current;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/core/components/navigation/use-responsive-tab-layout.ts(2 hunks)apps/web/core/components/sidebar/sidebar-wrapper.tsx(1 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/navigation/use-responsive-tab-layout.tsapps/web/core/components/sidebar/sidebar-wrapper.tsx
🧬 Code graph analysis (1)
apps/web/core/components/sidebar/sidebar-wrapper.tsx (2)
packages/propel/src/icons/actions/preferences-icon.tsx (1)
PreferencesIcon(6-13)apps/web/core/components/sidebar/sidebar-toggle-button.tsx (1)
AppSidebarToggleButton(7-23)
⏰ 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 (2)
apps/web/core/components/navigation/use-responsive-tab-layout.ts (1)
45-62: LGTM! This fixes a timing bug with ResizeObserver setup.The dependency array change from
[]to[container]ensures theResizeObserveris properly set up when the DOM element is attached. The previous code would only run once on mount, potentially before the ref was attached, causing the observer to never be initialized.apps/web/core/components/sidebar/sidebar-wrapper.tsx (1)
52-63: No UX impact on other sidebars from this conditional rendering.The Preferences and toggle buttons are only rendered when
title === "Projects", butSidebarWrapperis currently used in only one location withtitle="Projects". There are no other sidebars using this component with different title values, so this change does not break existing functionality. The conditional is scoped appropriately to the current usage pattern.Likely an incorrect or invalid review comment.
…dable-search-improvements-and-refactor
pushya22
deleted the
chore-expandable-search-improvements-and-refactor
branch
… focus restoration (makeplane#8207)
… focus restoration (makeplane#8207)
… focus restoration (makeplane#8207)
Description
This PR includes the following changes:
Type of Change
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.