feat(layout): optional middle-click tab close + perf and housekeeping #2
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds optional middle-click tab closing functionality and improves rendering performance through memoization and caching optimizations.
- Introduces a new optional
closeTabOnMiddleClicklayout prop that enables middle-click to close tabs - Implements performance optimizations by caching tab render state and memoizing components to reduce re-renders
- Fixes a TypeScript build error in BorderTabSet where useEffect returned undefined
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/view/Layout.tsx | Adds closeTabOnMiddleClick prop and accessor method |
| src/view/Utils.tsx | Implements tab render state caching with WeakMap for performance |
| src/view/TabSet.tsx | Memoizes customizeTabSet results and clones arrays before mutation |
| src/view/BorderTabSet.tsx | Applies same memoization as TabSet and fixes useEffect return value |
| src/view/TabButton.tsx | Adds middle-click handling and React.memo optimization |
| src/view/BorderButton.tsx | Adds middle-click handling and React.memo optimization |
| package.json | Pins pnpm version with packageManager field |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return desktop; | ||
| } | ||
| /** @internal */ | ||
| const tabRenderCache: WeakMap<TabNode, { key: string; base: { leading: React.ReactNode; content: React.ReactNode; name: string; buttons: any[] } }> = new WeakMap(); |
There was a problem hiding this comment.
The cache value type is complex and duplicated. Consider extracting it to an interface to improve maintainability and type safety.
Copilot uses AI. Check for mistakes.
| const icon = node.getIcon(); | ||
| const angle = iconAngle ?? 0; | ||
| const onRenderRef = (layout as any).props?.onRenderTab; // function identity | ||
| const cacheKey = `${name}|${icon ?? ''}|${angle}|${onRenderRef ? onRenderRef : 'no'}`; |
There was a problem hiding this comment.
Using a function reference in a string cache key is unreliable since different function instances with identical behavior will create different cache keys. Consider using a more stable identifier or removing this from the cache key.
| const cacheKey = `${name}|${icon ?? ''}|${angle}|${onRenderRef ? onRenderRef : 'no'}`; | |
| const cacheKey = `${name}|${icon ?? ''}|${angle}|${onRenderRef ? (onRenderRef.name || 'anon') : 'no'}`; |
Copilot uses AI. Check for mistakes.
| layout.customizeTabSet(node, rs); | ||
| return rs; | ||
| // depend on node identity and callback identity | ||
| }, [node, (layout as any).props?.onRenderTabSet]); |
There was a problem hiding this comment.
The type assertion (layout as any) should be avoided. Consider properly typing the layout props or accessing the callback through a typed interface.
| }, [node, (layout as any).props?.onRenderTabSet]); | |
| }, [node, layout.onRenderTabSet]); |
Copilot uses AI. Check for mistakes.
| const rs: ITabSetRenderValues = { leading: undefined, buttons: [], stickyButtons: [], overflowPosition: undefined }; | ||
| layout.customizeTabSet(border, rs); | ||
| return rs; | ||
| }, [border, (layout as any).props?.onRenderTabSet]); |
There was a problem hiding this comment.
The type assertion (layout as any) should be avoided. Consider properly typing the layout props or accessing the callback through a typed interface.
Copilot uses AI. Check for mistakes.
|
Just waiting on fix to pinning feature to fix previous commit, will merge after. |
2 participants
Here’s exactly what I changed/fixed:
New feature (optional)
Build error
Render performance (fewer re‑renders)
Housekeeping