design/PINNED_TABS_REVIEW.md
Rama main · réplica de lectura del repositorio original.
# Pinned Tabs Branch Review
Source-of-truth checklist for all findings from the pinned tabs feature branch review.
## Bugs
- [x] **B1** Ephemeral tab leak in `pinned-tabs:remove` IPC handler
- `src/main/ipc/browser/pinned-tabs.ts:199-202` vs `274-286`
- The `pinned-tabs:remove` handler only calls `pinnedTabsController.remove()` but does NOT destroy the associated ephemeral tab. The context menu "Unpin" action does destroy it. Orphaned ephemeral tabs leak because they skip the archive check in tabs-controller.
- Fix: Destroy the associated ephemeral tab in the `remove` handler, matching the context menu behavior.
- [x] **B2** Side effect inside `setSlots` state updater in slot machine
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/slot-machine/main.tsx:189-197`
- `openWinnerTabs(winners)` is called inside a `setSlots` updater function. React may invoke updaters multiple times in StrictMode/concurrent mode, causing duplicate tab opens.
- Fix: Use a ref to track current slots and move the side effect into a separate callback or effect.
- [x] **B3** `useEffect` dependency uses boolean expression instead of value
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/carousel.tsx:58`
- Dependency is `pages.length > 0` (boolean), not `pages.length`. Effect won't re-run when page count changes (e.g., 2->3).
- Fix: Use `pages.length` as the dependency.
- [x] **B4** Misleading error message in `useBrowserSidebar`
- `src/renderer/src/components/browser-ui/browser-sidebar/provider.tsx:111`
- Error says "AdaptiveTopbarProvider" but should say "BrowserSidebarProvider". Copy-paste error.
- Fix: Correct the error message string.
- [x] **B5** Non-unique HTML `id` attributes in mapped components
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/pinned-tab-button.tsx:230-231`
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/slot-machine/main.tsx:62-63`
- `id="overlay-overlay"` and `id="overlay"` used inside `.map()` loops, violating HTML uniqueness requirement.
- Fix: Switch from `id` to `className` (or use `pin.css` class selectors instead).
## Code Duplication
- [x] **D1** `isPinnedTabSource()` type guard duplicated 4 times
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/normal/pin-grid.tsx:20`
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/pinned-tab-button.tsx:40`
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/tab-group.tsx:16`
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/tab-drop-target.tsx:12`
- Fix: Extract to a shared `drag-utils.ts` file alongside the components.
- [x] **D2** `isTabGroupSource()` type guard duplicated 2 times
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/normal/pin-grid.tsx:16`
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/pinned-tab-button.tsx:44`
- Fix: Colocate with `isPinnedTabSource` in the shared utility.
- [x] **D3** `rgba()` + `generateBorderGradient()` duplicated verbatim across 2 files
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/pinned-tab-button.tsx:14-31`
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/slot-machine/main.tsx:12-26`
- Fix: Extract to a shared `pin-grid/utils.ts` module.
- [x] **D4** Entire pin button visual (overlay divs, dual favicon, active styles) copy-pasted into `SlotButton`
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/pinned-tab-button.tsx:213-243`
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/slot-machine/main.tsx:49-76`
- Fix: Extract a shared `PinVisual` presentation component.
- [x] **D5** IPC: "create tab + associate + activate" pattern duplicated 3 times
- `src/main/ipc/browser/pinned-tabs.ts:139-151`, `183-193`, `231-245`
- Fix: Extract a `createAndAssociatePinnedTab()` helper function.
- [x] **D6** IPC: "move ephemeral tab to current space" pattern duplicated in click + double-click
- `src/main/ipc/browser/pinned-tabs.ts:128-131`, `173-175`
- Fix: Extract a `moveEphemeralTabToCurrentSpace()` helper.
- [x] **D7** IPC: `double-click` handler is nearly identical to `click`
- `src/main/ipc/browser/pinned-tabs.ts:112-194`
- Differs only by navigating the associated tab to `defaultUrl` first.
- Fix: Refactor `click` to accept a `navigateToDefault` option, or have `double-click` call into `click` after navigation.
- [x] **D8** Mouse-press animation pattern duplicated 3 times in navigation-controls
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/navigation-controls.tsx:66-91`, `153-178`, `231-249`
- Fix: Extract a `usePressAnimation(iconRef)` custom hook.
- [x] **D9** `GoBackButton` and `GoForwardButton` are structurally near-identical
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/navigation-controls.tsx:55-139` vs `142-226`
- Same structure: icon ref, press handling, context menu, popover, entry list. Only differ by direction and icon.
- Fix: Create a single `NavigationButton` component parameterized by direction.
- [x] **D10** Tab serialization + `markDirty` pattern repeated 5 times in tabs-controller
- `src/main/controllers/tabs-controller/index.ts` — multiple event handlers
- Fix: Extract a private `persistTab(tab: Tab)` method.
## Architectural Improvements
- [x] **A1** `PinnedTabsController` uses custom Set-based listener instead of `TypedEventEmitter`
- `src/main/controllers/pinned-tabs-controller/index.ts:49-83`
- Every other controller extends `TypedEventEmitter`. This one uses a bespoke system, missing `once()`, `removeListener()`, etc.
- Fix: Extend `TypedEventEmitter<{ changed: [] }>` and use `emit`/`on` pattern.
- [x] **A2** `normalizePositions` runs in a separate DB transaction from its caller
- `src/main/controllers/pinned-tabs-controller/index.ts:320-341`
- Called from `create`, `remove`, and `reorder` — each already does a DB write, then `normalizePositions` opens another transaction.
- Fix: Wrap the outer operation + `normalizePositions` in a single atomic transaction.
- [x] **A3** Identity mapping functions add boilerplate without transformation
- `src/main/controllers/pinned-tabs-controller/index.ts:9-27`
- `pinnedTabRowToPersistedData` and `persistedDataToPinnedTabInsert` manually copy fields between structurally identical types.
- Fix: Use spread or direct assignment since the types have the same shape.
- [x] **A4** Controller not imported through `controllers/index.ts` barrel file
- `src/main/browser.ts:17` imports `pinned-tabs-controller` directly instead of through `controllers/index.ts` like all others.
- Fix: Add the import to the barrel file.
- [x] **A5** Redundant `pendingChange` flag in IPC debounce
- `src/main/ipc/browser/pinned-tabs.ts:11-22`
- The `pendingChange` boolean is always `true` when the timeout fires. A simple trailing-edge debounce suffices.
- Fix: `let timeout; function schedule() { clearTimeout(timeout); timeout = setTimeout(process, 80); }`
## Dead Code / Cleanup
- [x] **C1** `intervalRef` in SlotMachinePinGrid is never written to
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/slot-machine/main.tsx:150`
- `clearInterval(intervalRef.current)` on unmount is always a no-op.
- Fix: Remove the ref and its cleanup.
- [x] **C2** `triggerRef` in GoBack/GoForward buttons is assigned but never read
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/navigation-controls.tsx:67,154`
- Fix: Remove the refs.
- [x] **C3** `NAVIGATION_ANIMATION_ENABLED = true` — dead toggle constant
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/navigation-controls.tsx:16`
- Fix: Remove the constant and its conditionals.
- [x] **C4** `{false && <UpdateEffect />}` — permanently disabled dead code
- `src/renderer/src/components/browser-ui/main.tsx:295`
- Fix: Remove the expression and the unused import.
- [x] **C5** `"use client"` directive in an Electron/Vite app
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/sidebar-scroll-area.tsx:1`
- No Next.js — directive has no effect.
- Fix: Remove the directive.
- [x] **C6** Commented-out fullscreen implementation block
- `src/renderer/src/components/browser-ui/main.tsx:208-216`
- Fix: Remove the commented-out code.
- [x] **C7** `remove` function exported from pinned-tabs-provider but never consumed
- `src/renderer/src/components/providers/pinned-tabs-provider.tsx:16,78,119`
- No component calls `usePinnedTabs().remove()`. Removal goes through `showContextMenu`.
- Fix: Keep if intentionally for future use, or remove to reduce API surface.
## Minor Improvements
- [x] **M1** Optimistic reorder spreads non-matching tabs unnecessarily
- `src/renderer/src/components/providers/pinned-tabs-provider.tsx:98`
- `{ ...t }` on non-matching branch creates new objects for every tab, breaking reference equality.
- Fix: Use `t` (no spread) for non-matching entries.
- [x] **M2** `handleReorder` and `handleCreateFromTab` are identity wrappers
- `src/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/normal/pin-grid.tsx:214-228`
- Wrap already-stable `useCallback` from the provider with no additional logic.
- Fix: Pass `reorder` and `createFromTab` directly as props.
- [x] **M3** Two `useEffect` calls in provider (initial fetch + subscribe) could be merged
- `src/renderer/src/components/providers/pinned-tabs-provider.tsx:45-57`
- Subscribe-first-then-fetch pattern closes the race window between initial fetch resolving and listener registration.
- Fix: Merge into a single effect.
- [x] **M4** Misleading comment about `loadAll()` having an "async signature"
- `src/main/browser.ts:23-25`
- The method is synchronous. Comment is stale.
- Fix: Update the comment.
- [x] **M5** `useMemo` used for side effect (setting a ref) in sidebar provider
- `src/renderer/src/components/browser-ui/browser-sidebar/provider.tsx:133-135`
- Fix: Use `useRef(getInitialSidebarSize())` directly in the declaration.