feat: implement Tab Service v2 and replace old tab manager#276
Open
devin-ai-integration[bot] wants to merge 98 commits into
Open
feat: implement Tab Service v2 and replace old tab manager#276devin-ai-integration[bot] wants to merge 98 commits into
devin-ai-integration[bot] wants to merge 98 commits into
Conversation
- Core entities: Tab (OOP, WebContentsView lifecycle, TypedEventEmitter), TabLayoutNode (replaces old TabGroup for visual display grouping), PinnedTab (first-class citizen with per-space associations) - Layout management: TabLayout (per-window active/focused state), TabPositioner (float-point ordering for efficient insertion) - TabService orchestrator: unified tab creation, activation, movement, pinned tab operations, and event emission - Persistence: TabPersistenceService with dirty tracking and batched flushing every 2s - IPC: TabIPC with debounced structural/content updates, 20+ handlers - Shared types: TabOwnerRef union (normal/pinned/bookmark) for future-proofing bookmarks feature - Preload API factory: createTabServicePreloadAPI for renderer bridge - React provider: TabServiceProvider with layout node computation, pinned tabs state, and focused tab tracking - FlowTabServiceAPI interface for renderer consumption Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Contributor
Build artifacts for all platforms are ready! 🚀Download the artifacts for: One-line installer (Unstable):bunx flow-debug-build@1.2.1 --open 26471187611(execution 26471187611 / attempt 1) |
Member
|
@greptile review (aside) |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- TabLayoutNode: track destroy listeners per tab and clean them up on removeTab() and destroy() to prevent crashes from stale callbacks - TabLayoutNode: guard onDestroyed callback with isDestroyed check - TabService.unpinToTabList: collect affected window IDs before calling destroy() which clears associations, ensuring structural changes are properly emitted to the renderer Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
Greptile SummaryThis PR replaces the old procedural
Confidence Score: 4/5Safe to merge after addressing the missing post-createTab destroy guard in clickPinnedTab; all other changes are well-structured and the previously-flagged regressions are resolved. The rewrite is architecturally sound and previously-flagged regressions are addressed. One new defect remains: the new-tab creation branch of clickPinnedTab skips the post-await stale check that the existing-tab branch already has, which can leave a pinned tab permanently unresponsive if the window is destroyed mid-flight. src/main/services/tab-service/tab-service.ts (new-tab path in clickPinnedTab, line 706) and src/main/services/tab-service/tab-lifecycle-timer.ts (discarded setInterval handle) Important Files Changed
Sequence DiagramsequenceDiagram
participant R as Renderer
participant IPC as TabIPC
participant TS as TabService
participant TLN as TabLayoutNode
participant Sync as TabSync
participant Persist as TabPersistenceService
R->>IPC: tab-service:pinned-tabs-click
IPC->>TS: clickPinnedTab(pinnedTabId, window)
alt existing associated tab
TS->>Sync: sendPlaceholderForTab(tab, oldWindow)
Note over Sync: capturePage() async
Sync-->>TS: screenshot sent
TS->>TLN: setActiveLayout(targetLayout)
TS->>TS: activateTab(existingTab)
else no associated tab
TS->>TS: createTab() async profile load
TS->>TLN: createSingleNode(tab)
TS->>TS: propagatePinnedTabNode(node, profileId)
TS->>TS: activateTab(tab)
end
TS->>IPC: emit structural-change windowId
IPC->>R: tab-service on-data-changed debounced 32ms
Note over Persist: Every 2 seconds
Persist->>Persist: flush snapshot dirty maps run transaction
alt transaction fails
Persist->>Persist: re-queue unmasked entries
end
Reviews (8): Last reviewed commit: "fixes" | Re-trigger Greptile |
Contributor
Author
Test Results — Regression TestingRan Flow Browser locally on All 6 tests passed
Important: Integration StatusThe new TabService v2 is architecturally complete but not yet integrated into the running application:
The existing Before the new TabService can be tested end-to-end, these integration steps are needed:
Evidence Screenshots
|
…, flush data-loss)
- wireTabEvents: look up layout dynamically from this.layouts instead of
capturing stale closure reference. Fixes broken cleanup after setWindow.
- wireTabEvents: detect wasActive even after the once('destroyed') listener
from TabLayoutNode.addTab has already removed the tab and auto-destroyed
the node. Checks activeNode.isDestroyed as fallback.
- TabPersistenceService.flush: clear dirty state only after successful
transaction to prevent data loss if the DB write throws.
Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
…nView WebContentsView does not have a destroy() method in Electron. The old tab controller correctly uses webContents.close() to tear down views. Fixed the same pattern in Tab Service v2. Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
Contributor
Author
End-to-End Integration Test ResultsTested TabService v2 IPC integration by calling All 7 tests passed after fixing one bug discovered during testing. Test Results
Bug Found & Fixed
Key EvidenceSidebar UI renders TabService v2 tabs (proves full renderer integration): closeTab works after fix (before: 2, after: 1): All operations complete without errors: Notes
|
- Migrate all main-process imports from tabsController to tabService - Migrate all renderer flow.tabs.*/flow.pinnedTabs.* to flow.tabService.* - Remove old preload tabs/pinnedTabs API - Remove old IPC handlers (tabs.ts, pinned-tabs.ts) - Remove old tabs-controller and pinned-tabs-controller directories - Remove old TabPersistenceManager (saving/tabs/index.ts) - Remove old shared flow interfaces (tabs.ts, pinned-tabs.ts) - Update DB schema to use new TabLayoutNodeMode type - Add setCurrentWindowSpace/handlePageBoundsChanged to TabService - Add removeLayoutNode to TabPersistenceService - Rewrite saving/tabs/restore.ts for new TabService API - Update app-menu-controller to use tabService.recentlyClosed Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
Without calling updateTabVisibility() and handlePageBoundsChanged(), switching tabs only changed internal state but didn't show/hide the actual Electron WebContentsViews. Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
devin-ai-integration
Bot
changed the title
activateTab/activateNode must notify the renderer of active state changes so the sidebar highlights the correct active tab. Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
…/update) - Added loadPinnedTabs() that reads from pinned_tabs table on startup - Added savePinnedTab() for upsert on create and favicon/position updates - Added deletePinnedTabFromDb() for remove and unpin operations - Added 'Pin Tab' context menu item for regular tabs - Wire updated event to persist favicon/position changes automatically - Called loadPinnedTabs() in initializeTabService() Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
- Add 'Sync Tabs Across Windows' feature (tab-sync.ts): - Moves active tab view to focused window when setting enabled - Shows screenshot placeholders in windows that lose the view - Relocates tabs from closing windows to surviving windows - Pinned tabs always sync across windows regardless of setting - Uses async queue with generation-based staleness detection - Fix pinned tabs across multiple windows via moveTabToWindowHook - Re-add web page right-click context menu (electron-context-menu): - Back/Forward/Reload, Open link in new tab/window - Copy/Paste/Cut/SelectAll, Search Google for selection - Save Image As, Copy Image/Image Address - View Page/Frame Source, Inspect Element - Extension context menu items - Add did-fail-load handler with error page navigation - Add devtools-open-url handler for opening URLs from DevTools - Add update-target-url handler for hover link URL preview - Clear placeholders when tabs are destroyed Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
…nd tabs - IPC: include all tabs in payload (pinned-tab-owned tabs visible to renderer) - IPC: pinned-tab-owned content updates broadcast to all windows (always-sync) - Renderer: filter pinned/bookmark-owned tabs from sidebar groups - Tab: add setFullScreen() with document.exitFullscreen() and event emission - Tab: route enter/leave-html-full-screen through setFullScreen() - Tab: call applyUrlBackground() after wakeUp() - TabService: exit fullscreen when tab is hidden or space changes - TabService: listen for fullscreen-changed to update bounds - TabService: listen for BrowserWindow leave-full-screen to reset tab state - TabService: add startBackgroundTasks() for space-deletion + auto-sleep/archive - TabService: respect makeActive option (background tabs don't activate) - TabService: add popup window reconciliation (auto-close empty popups) Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
Bug 1 - Tab sync clicking does nothing: - switch-to-tab IPC now detects cross-window requests and moves the tab to the requesting window before activating it - Added migrateTabBetweenLayouts() to move layout nodes between windows (previously only the view/layer moved, not the node) - moveTabToWindowIfNeeded now calls layout migration before setWindow Bug 2 - Archive Tab After too aggressive: - Fixed unit mismatch: lastActiveAt is in seconds (getCurrentTimestamp) but timer compared against Date.now() (milliseconds). The difference was always ~1.7 trillion, exceeding any archive threshold immediately. - Timer now uses seconds consistently (parseDurationToSeconds) - activateTab() now updates lastActiveAt on each tab switch Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
The archive/sleep timer should measure from when the user stopped viewing a tab. Previously lastActiveAt was only set on activation, so a tab viewed for hours would appear 'inactive since activation' when finally switched away from. Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
1. Focus loop: When a tab was migrated between windows, migrateTabBetweenLayouts destroyed the old node but left the focusedTabMap entry pointing at the migrated tab. The window-focus handler then found a 'focused tab' in the old window and moved it back, creating an infinite focus cycle. Fixed by clearing the focused tab reference when migrating. 2. Placeholder corners: Added overflow-hidden to BrowserContent container so the tab screenshot respects the parent's border-radius. Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
The focus handler previously used async moveTabToWindowIfNeeded which calls captureTabScreenshot(). During the async gap, Electron can fire a focus event for the other window (platform quirk when WebContentsViews move), incrementing _focusMoveGeneration and making the current move stale. Result: the move was cancelled and the tab never arrived. Fix: The focus handler now uses moveTabToWindowImmediate() which moves the tab synchronously without screenshot capture. The old window just shows empty content (no placeholder image) instead of a screenshot — acceptable trade-off for reliable tab movement. Also reverted the focusedTabMap clearing in migrateTabBetweenLayouts — the focused-tab reference is the window's 'memory' of what tab it was viewing, needed by the focus handler to pull it back. Additionally fixed placeholder rounded corners: container now uses rounded-md (6px) matching the actual tab view's setBorderRadius(6). Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
… focus handler Root cause of the infinite loop: 1. Focus handler moves tab from B to A 2. activateTab() emits 'active-changed' 3. 'active-changed' triggered relocateDisplacedTabs() 4. Both windows have the same tab in their focusedTabMap 5. relocateDisplacedTabs sees B 'wants' the tab and if B appears focused (Electron quirk during WebContentsView manipulation), it moves the tab back → triggering step 1 again Fix: - Removed relocateDisplacedTabs entirely. All tab moves are handled by explicit paths: focus handler, switch-to-tab IPC, moveTabToWindowHook (pinned tab click), and space-change handler. The reactive 'catch-all' was redundant and caused fights with these explicit handlers. - Focus handler now uses the async moveTabToWindowIfNeeded (screenshot → move → placeholder) serialized through runTabSyncMutation, with a 150ms debounce to ignore spurious focus events from Electron's WebContentsView manipulation. - Removed moveTabToWindowImmediate (replaced by proper async approach). Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
The focus handler now programmatically calls window.focus() AFTER the tab has been moved, activated, and made visible. This ensures the correct window stays in front even when Electron briefly shifts focus during WebContentsView manipulation. Also increased debounce to 200ms to better absorb spurious events. Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
Three changes to fix focus cycling: 1. Set debounce timestamp BEFORE queuing the async mutation (not just after). Previously, during the ~100ms screenshot capture, a spurious focus event for the other window could queue a competing mutation that would move the tab back. Now the debounce gate is locked immediately when any focus-triggered move starts. 2. activateTab() now calls tab.layer.focus() after making the tab visible. This tells the LayerManager to give the tab's webContents input focus in the target window. Without this, after the old window's reallocateFocus fires (focusing its browserUI webContents), the target window had no explicitly focused tab layer, leaving it in an ambiguous focus state. 3. Removed window.browserWindow.focus() call (user request) — the correct approach is to focus the tab's layer, not the window itself. Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
Root cause: when a tab moves between windows, the old window's LayerManager calls reallocateFocus() (triggered by the focused layer being hidden/removed). This calls webContents.focus() on the next available layer, which can steal OS focus to the background window, causing infinite focus cycling. Fix: LayerManager.reallocateFocus() now checks if its window is focused before executing. If not focused, it sets a pending flag. When the window later gains focus, the deferred reallocation runs once — regardless of how many times it was requested while unfocused. This is a clean, general fix that prevents ANY background window from stealing focus via webContents.focus(). The tab-sync debounce hack is removed since it's no longer needed. Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
When layer.focus() succeeds, there's no need for a deferred reallocateFocus — focus has already been assigned to the correct layer. Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
- Remove stale position cache in TabLayoutNode (always recompute) - Include layout-derived spaces in IPC payload (pinned tab coverage) - Validate same-space/window in createLayoutNode - Hide pinned tabs in old active node on space switch - Use _pipCount instead of O(n) scan for PiP check in space switch - Only hide tabs in batchMoveTabs when actually leaving source layout - Wrap restore batch in try/finally for endBatch safety - Guard clickPinnedTab after async sendPlaceholderForTab Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
Member
|
@greptile @coderabbitai review (aside) |
Contributor
|
✅ Actions performedReview triggered.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/services/tab-service/tab-service.ts (1)
754-760:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror the post-await destruction guard in
doubleClickPinnedTab.This path still calls
existingTab.setWindow(window)afterawait sendPlaceholderForTab(...). If the tab or target window is destroyed during that await, you'll mutate stale state or throw on a user-driven close race.💡 Minimal fix
if (existingTab.getWindow().id !== window.id) { const oldWindow = existingTab.getWindow(); await sendPlaceholderForTab(existingTab, oldWindow); + if (existingTab.isDestroyed || window.destroyed) return true; existingTab.setWindow(window); node.setActiveLayout(targetLayout); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/services/tab-service/tab-service.ts` around lines 754 - 760, The code calls existingTab.setWindow(window) and node.setActiveLayout(targetLayout) after awaiting sendPlaceholderForTab(existingTab, oldWindow), which can race with tab/window destruction; mirror the post-await destruction guard used in doubleClickPinnedTab by re-checking that existingTab and window are still valid (not destroyed/closed and still attached to expected parents) before mutating them—specifically, after the await verify existingTab is not disposed and existingTab.getWindow().id still equals oldWindow.id (or that target window still exists) and only then call existingTab.setWindow(window) and node.setActiveLayout(targetLayout); otherwise abort the move.
♻️ Duplicate comments (1)
src/main/services/tab-service/tab-service.ts (1)
1261-1265:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't rely on
WeakMapGC to retire oldWebContentsmappings.A
WeakMaponly drops an entry after the oldWebContentsbecomes unreachable.getTabByWebContents(oldWebContents)can still resolve to this tab while any caller is holding that stale object, so sleep/view-replacement paths need an explicitdelete(...)for the retired key.#!/bin/bash set -euo pipefail printf '\n== webContentsIndex mutation sites ==\n' rg -n -C2 'webContentsIndex\.(set|delete)\(' src/main/services/tab-service printf '\n== WebContents lifecycle sites on Tab/TabService ==\n' rg -n -C3 '\b(teardownView|wakeUp|sleep|webContents\b|window-changed|destroyed)\b' \ src/main/services/tab-service \ src/main/services/tab-service/coreExpected result: you'll find
webContentsIndex.set(...)sites but no matchingwebContentsIndex.delete(...)on teardown/replacement paths, which confirms the stale reverse-index window.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/services/tab-service/tab-service.ts` around lines 1261 - 1265, The webContentsIndex WeakMap is left pointing at retired WebContents objects, so update paths that replace or tear down a tab's webContents (e.g., the asleep/wakeUp branch shown, sleep(), teardownView(), any view-replacement/destroy handlers) must explicitly remove the old mapping before or when assigning a new one; locate webContentsIndex.set(...) usages and add this.webContentsIndex.delete(oldWebContents) (using the previous tab.webContents reference) in wakeUp, sleep, teardownView, and any window-changed/destroyed handlers so getTabByWebContents cannot return stale mappings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/services/tab-service/tab-service.ts`:
- Around line 754-760: The code calls existingTab.setWindow(window) and
node.setActiveLayout(targetLayout) after awaiting
sendPlaceholderForTab(existingTab, oldWindow), which can race with tab/window
destruction; mirror the post-await destruction guard used in
doubleClickPinnedTab by re-checking that existingTab and window are still valid
(not destroyed/closed and still attached to expected parents) before mutating
them—specifically, after the await verify existingTab is not disposed and
existingTab.getWindow().id still equals oldWindow.id (or that target window
still exists) and only then call existingTab.setWindow(window) and
node.setActiveLayout(targetLayout); otherwise abort the move.
---
Duplicate comments:
In `@src/main/services/tab-service/tab-service.ts`:
- Around line 1261-1265: The webContentsIndex WeakMap is left pointing at
retired WebContents objects, so update paths that replace or tear down a tab's
webContents (e.g., the asleep/wakeUp branch shown, sleep(), teardownView(), any
view-replacement/destroy handlers) must explicitly remove the old mapping before
or when assigning a new one; locate webContentsIndex.set(...) usages and add
this.webContentsIndex.delete(oldWebContents) (using the previous tab.webContents
reference) in wakeUp, sleep, teardownView, and any window-changed/destroyed
handlers so getTabByWebContents cannot return stale mappings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 835e8e2b-6253-474a-bcdf-c94e81335569
📒 Files selected for processing (5)
src/main/saving/tabs/restore.tssrc/main/services/tab-service/core/tab-layout-node.tssrc/main/services/tab-service/ipc/tab-ipc.tssrc/main/services/tab-service/layout/tab-layout.tssrc/main/services/tab-service/tab-service.ts
Mirror the isDestroyed/window.destroyed check already present in clickPinnedTab after sendPlaceholderForTab — prevents mutating stale state if tab or window is destroyed during capturePage(). Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
- Call notifyExtensionsOfChanges on every tab 'updated' event (title, url, muted, audible, isLoading, asleep, PiP state changes) - Add notifyIndexChanges helper to notify all tabs in a window+profile when their index may have shifted - Call notifyIndexChanges after: tab creation, tab destruction, tab window-change (both old and new window), moveTabToSpace, batchMoveTabs - Add migration checklist doc comparing old Tab Manager vs new Tab Service Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
The old Tab class emitted 'tab-updated' in setupTabLevelListeners() on every 'updated' event. This was lost during the Tab Service v2 migration and has now been restored via notifyExtensionsOfChanges(). Updated the checklist to accurately reflect this was a regression fix, not new functionality. Also fixes markdown table formatting for CI. Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
The previous implementation iterated layout nodes sorted by node.position (derived from Math.min of tabs' positions). This could produce non-deterministic results when nodes from different spaces shared the same position value, or when pinned tab nodes were filtered out by the getWindow().id check. Now directly queries getTabsInWindowSpace per layout and sorts by tab.position, which matches the renderer's sidebar ordering and guarantees unique indices within a window+profile. Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
Comprehensive guide for agents working in this directory. Covers architecture, class relationships, key patterns (extension notifications, multi-layout membership, IPC caching, tab sync), common pitfalls, and file overview. Includes maintenance rule requiring agents to keep this file updated as code changes. Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
1. Simplified getTabsInWindowProfile to just filter getTabsInWindow by profileId and sort by position. The previous layout-based approach (iterating getTabsInWindowSpace per layout) was failing because pinned tabs and cross-space scenarios produced inconsistent results. The extension API index is simply position-order among all tabs in the same window+profile regardless of space. 2. Added notifyIndexChanges call to moveTab (drag-drop reorder) so the extension system's cached tab details are refreshed after reorders. Note: chrome.tabs.onUpdated doesn't fire for index-only changes (per Chrome API spec), but the cached index is updated correctly for subsequent chrome.tabs.get/query calls. Co-Authored-By: Evan <47493765+iamEvanYT@users.noreply.github.com>
Member
|
@cursor review |
Member
|
@cursor @greptile @coderabbitai review (aside) |
|
Taking a look!   |
Contributor
|
✅ Actions performedFull review triggered. |
Member
|
bugbot run |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/services/tab-service/core/web-context-menu.ts (1)
40-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve the target window from
tabat click time.
createNewTab()closes over thewindowthat was current when the context menu was installed. Pinned/STAW tabs can later move to anotherBrowserWindow, so actions like “Open Link in New Tab” will keep using the old window/currentSpace unless this resolves fromtab.getWindow()instead.💡 Proposed fix
export function createWebContextMenu(tab: Tab, window: BrowserWindow) { @@ const createNewTab = async (url: string, overrideWindow?: BrowserWindow) => { - const targetWindow = overrideWindow ?? window; + const targetWindow = overrideWindow ?? tab.getWindow(); const spaceId = targetWindow.currentSpaceId ?? tab.spaceId; if (!spaceId) return; const newTab = await tabService.createTab(targetWindow.id, tab.profileId, spaceId, undefined, { url }); tabService.activateTab(newTab); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/services/tab-service/core/web-context-menu.ts` around lines 40 - 58, The createNewTab closure currently captures the initial window variable so actions like "Open Link in New Tab" keep using the old BrowserWindow; modify createNewTab in createWebContextMenu to resolve the target window at click time by calling tab.getWindow() (or the Tab instance method that returns its current BrowserWindow) and fall back to the passed overrideWindow or the original window only if getWindow() is null; ensure you null-check the resolved window and use its currentSpaceId (or tab.spaceId) before calling tabService.createTab and tabService.activateTab so the new tab opens in the tab's current BrowserWindow/space.
♻️ Duplicate comments (2)
docs/tab-layout-refactor-plan.md (1)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifiers to fenced code blocks.
These fences still omit a language token, which keeps
MD040active. Please annotate each block (for exampletext,typescript).Also applies to: 26-26, 45-45, 135-135, 148-148, 157-157, 170-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/tab-layout-refactor-plan.md` at line 5, Several fenced code blocks use bare triple backticks (```) which trigger MD040; update each triple-backtick fence in this document to include an explicit language identifier (for example ```text or ```typescript) so Markdown linter recognizes the language. Locate every instance of the plain ``` fence (including the ones noted in the review) and replace it with an appropriate token for the block content (e.g., ```text for plain output snippets, ```typescript for TS examples) to silence MD040 and improve syntax highlighting.src/renderer/src/components/providers/pinned-tabs-provider.tsx (1)
79-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
positionin unpin flow.Line 79-Line 80 drops the optional
positionargument even though the context contract includes it, so caller-specified unpin placement is lost.Suggested fix
-const unpinToTabList = useCallback(async (pinnedTabId: string) => { - return flow.tabService.unpinToTabList(pinnedTabId); +const unpinToTabList = useCallback(async (pinnedTabId: string, position?: number) => { + return flow.tabService.unpinToTabList(pinnedTabId, position); }, []);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/providers/pinned-tabs-provider.tsx` around lines 79 - 80, The unpinToTabList callback drops the optional position argument from the context contract; update the unpinToTabList signature to accept the optional position (e.g., unpinToTabList(pinnedTabId: string, position?: number)) and forward it to the underlying service by calling flow.tabService.unpinToTabList(pinnedTabId, position) so caller-specified unpin placement is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/controllers/loaded-profiles-controller/index.ts`:
- Around line 146-147: The code currently falls back to
browserWindowsController.getWindows()[0] when window is missing, which may open
tabs in the wrong profile; instead resolve or create a window scoped to the
given profileId before calling createTab: lookup an existing window for
profileId via browserWindowsController.getWindows().find(w => w.profileId ===
profileId) (or use a browserWindowsController.getWindowForProfile(profileId)
helper), and if none exists create a new profile-scoped window before assigning
targetWindow (replace use of the global window ||
browserWindowsController.getWindows()[0] with this profile-scoped resolution) so
createTab is always invoked in the correct profile context.
In `@src/main/saving/tabs/restore.ts`:
- Around line 118-138: restoreLayoutNodes currently translates
nodeData.tabUniqueIds to tabIds but drops nodeData.frontTabUniqueId, losing
front-tab state; map nodeData.frontTabUniqueId to a frontTabId via
uniqueIdToTabId (similar to the tabIds loop) and pass that frontTabId into
tabService.createLayoutNode (or extend createLayoutNode's signature to accept an
optional frontTabId) so recreated layout nodes preserve the front tab; update
restoreLayoutNodes (and createLayoutNode implementation/signature) to accept and
use the restored frontTabId when available.
In `@src/main/services/tab-service/AGENTS.md`:
- Around line 13-18: The markdown file contains unlabeled fenced code blocks
that trigger MD040; update each unlabeled triple-backtick block in AGENTS.md to
use a language identifier (use "text") — specifically the blocks containing the
short key/value list (tabService → TabService ...), the "Tab property changes →
Tab emits 'updated' → wireTabEvents handler →" block, and the directory listing
block starting with "tab-service/" (and any other unlabeled fences such as those
noted around 51-55 and 115-138) so they read ```text ... ``` instead of plain
```; keep the block contents unchanged.
- Around line 39-45: The Supporting Modules table rows in AGENTS.md are
malformed because several rows only provide two cells while the header requires
three (Module | File | Purpose); update each listed row (e.g., entries for
tab-sync.ts, tab-lifecycle-timer.ts, tab-context-menus.ts, web-context-menu.ts,
save-image-as.ts) so they contain three pipe-separated columns matching the
header (Module, File, Purpose) and ensure the separator row (| --- | --- | ---
|) is present; keep the module name (e.g., `tab-sync.ts`) in the first column,
the file path/details in the second, and the purpose text in the third for each
row.
In `@src/main/services/tab-service/core/tab-context-menus.ts`:
- Around line 145-148: The context menu currently uses hasURL to enable both
"Pin Tab" and "Unpin Tab" (new MenuItem with label based on isPinned), which
disables "Unpin Tab" for pinned tabs with no URL; change the enabled logic so
that when isPinned is true the menu item is always enabled (e.g. enabled =
isPinned ? true : hasURL) so unpinning remains available; update the MenuItem
creation (refer to the MenuItem, isPinned, hasURL, click symbols) accordingly.
- Around line 93-100: The duplicate-tab creation call to
tabService.createTab(...) currently only attaches .then(...) and lacks rejection
handling, risking unhandled promise rejections; update the call site to handle
failures by either awaiting the promise inside an async function with try/catch
or by chaining a .catch(...) to tabService.createTab(window.id, tab.profileId,
targetSpaceId, undefined, { url: tab.url, position: tab.position + 0.5 }) and
log or surface the error, ensuring normalizePositions(window.id, targetSpaceId)
runs only on success (or handle recovery on failure) so failures are handled
deterministically.
In `@src/main/services/tab-service/index.ts`:
- Around line 54-61: The init guard tabServiceInitialized is set before running
startup steps so any thrown error will leave the guard true and block future
retries; change the flow in the initializer (the function that contains
tabService.loadPinnedTabs(), tabService.startBackgroundTasks(),
tabPersistenceService.start(), tabIPC.initialize(), initTabSync()) to run those
startup calls first (optionally wrapped in a try/catch), and only set
tabServiceInitialized = true after all calls complete successfully (or set it
inside the try after successful completion and keep it false on error so
subsequent initializeTabService() attempts can retry).
In `@src/main/services/tab-service/layout/tab-layout.ts`:
- Around line 273-302: The registerNode() method attaches 'tab-added',
'tab-removed' and 'destroyed' listeners to TabLayoutNode but
removeNodeWithoutDestroy() does not detach them, so nodes moved to another
layout still trigger updates on this layout; fix by storing the bound handlers
(e.g. in a small Map keyed by node.id or properties on the node) when calling
node.on in registerNode(), and in removeNodeWithoutDestroy() (and any other
unregister path around the 320-333 area) call node.off('tab-added', handler),
node.off('tab-removed', handler) and node.off('destroyed', handler) to fully
remove those listeners and prevent stale tabToNode updates and spurious
'layout-node-destroyed' emits.
In `@src/main/services/tab-service/layout/tab-positioner.ts`:
- Around line 47-55: The midpoint calculation uses the possibly-stale
tab.position; instead, derive the baseline from the sorted array: locate the
index via sorted.findIndex and use sorted[index].position as the current
position baseline (or fall back to tab.position if index === -1), then compute
the midpoint with sorted[index + 1].position; update the early-return branch and
the final return to reference sorted[index].position rather than tab.position so
insert positions are computed from the sorted list.
In `@src/main/services/tab-service/tab-service.ts`:
- Around line 1021-1025: The active-changed listener currently always calls
updateTabVisibility(wId, sId) which ignores layout.visibility and causes tabs in
hidden layouts to be force-shown; fix by making the visibility check explicit:
either have the layout.on("active-changed", ...) handler only call
updateTabVisibility when layout.visible is true, or modify updateTabVisibility
to early-return when the originating layout (or its visible flag) is false so
activateTab() and pinned/STAW activations in non-current spaces do not reveal
tabs until the user actually switches spaces; apply the same guard to the other
active-changed handlers (the similar block around the activateTab/pinned logic)
so hidden layouts never trigger UI updates.
- Around line 904-960: When removing/destroying the source node in
moveTabToSpace, ensure the source layout selects a replacement active node if
the moved tab was the currently focused tab: after the removal/destroy logic in
moveTabToSpace, check if sourceLayout.getFocusedTab()?.id === tab.id and if so
call a layout API to pick/activate a replacement (e.g.
sourceLayout.selectReplacementActiveNode() or
sourceLayout.focusNextAvailableTab()); alternatively call
sourceLayout.removeFocusedTab() immediately for that specific sourceLayout and
then invoke its replacement-selection routine so the visible window isn’t left
blank. Ensure you use the existing symbols moveTabToSpace, sourceLayout,
getNodeForTab, getFocusedTab and removeFocusedTab to locate where to add this
behavior.
- Around line 1493-1533: handleNewTabRequested currently always uses
sourceTab.spaceId when creating the new tab, which can be stale for pinned/STAW
tabs; instead query the target window's active space and use that for the new
tab's spaceId. Update the function to get the target window object for windowId
(e.g., via browserWindowsController.get(windowId) or similar API), read its
active/current space id (e.g., targetWindow.getCurrentSpace() or
targetWindow.activeSpaceId), and pass that space id into createTabInternal
(falling back to sourceTab.spaceId if no active space is available); also ensure
when creating a popup you still call popupWindow.setCurrentSpace(...) using the
resolved active space where appropriate.
- Around line 597-621: createPinnedTabFromTab currently creates a PinnedTab but
never propagates the existing layout node, so clicks later rely on stale
locations; update createPinnedTabFromTab to copy the live tab's layout node into
pinnedTab.layoutNode (from the source Tab) and call
propagatePinnedTabNode(pinnedTab, /* appropriate args if required */) the same
way clickPinnedTab does, ensuring the new pinned tab is registered in the layout
before saving/emit events (refer to createPinnedTabFromTab, clickPinnedTab,
pinnedTab.layoutNode, and propagatePinnedTabNode to locate the spots to modify).
In
`@src/renderer/src/components/browser-ui/browser-sidebar/_components/tab-group.tsx`:
- Around line 126-129: The context-menu click handler calls
flow.tabService.showContextMenu(tab.id) without verifying tab.id; add a guard in
that handler (the anonymous React.MouseEvent handler around
flow.tabService.showContextMenu) to return early if tab.id is null/undefined (or
otherwise invalid) before invoking flow.tabService.showContextMenu, mirroring
adjacent handlers that check the id so the service never receives an invalid id.
---
Outside diff comments:
In `@src/main/services/tab-service/core/web-context-menu.ts`:
- Around line 40-58: The createNewTab closure currently captures the initial
window variable so actions like "Open Link in New Tab" keep using the old
BrowserWindow; modify createNewTab in createWebContextMenu to resolve the target
window at click time by calling tab.getWindow() (or the Tab instance method that
returns its current BrowserWindow) and fall back to the passed overrideWindow or
the original window only if getWindow() is null; ensure you null-check the
resolved window and use its currentSpaceId (or tab.spaceId) before calling
tabService.createTab and tabService.activateTab so the new tab opens in the
tab's current BrowserWindow/space.
---
Duplicate comments:
In `@docs/tab-layout-refactor-plan.md`:
- Line 5: Several fenced code blocks use bare triple backticks (```) which
trigger MD040; update each triple-backtick fence in this document to include an
explicit language identifier (for example ```text or ```typescript) so Markdown
linter recognizes the language. Locate every instance of the plain ``` fence
(including the ones noted in the review) and replace it with an appropriate
token for the block content (e.g., ```text for plain output snippets,
```typescript for TS examples) to silence MD040 and improve syntax highlighting.
In `@src/renderer/src/components/providers/pinned-tabs-provider.tsx`:
- Around line 79-80: The unpinToTabList callback drops the optional position
argument from the context contract; update the unpinToTabList signature to
accept the optional position (e.g., unpinToTabList(pinnedTabId: string,
position?: number)) and forward it to the underlying service by calling
flow.tabService.unpinToTabList(pinnedTabId, position) so caller-specified unpin
placement is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 094c1105-a791-4e93-96ca-e55e5650b3b8
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (91)
docs/tab-layout-refactor-plan.mddocs/tab-service-architecture.mddocs/tab-service-migration-checklist.mdpackage.jsonsrc/main/app/basic-auth.tssrc/main/app/urls.tssrc/main/browser.tssrc/main/controllers/app-menu-controller/index.tssrc/main/controllers/app-menu-controller/menu/helpers.tssrc/main/controllers/app-menu-controller/menu/items/file.tssrc/main/controllers/app-menu-controller/menu/items/tabs.tssrc/main/controllers/handoff-controller/index.tssrc/main/controllers/index.tssrc/main/controllers/loaded-profiles-controller/index.tssrc/main/controllers/pinned-tabs-controller/index.tssrc/main/controllers/quit-controller/handlers/before-quit.tssrc/main/controllers/sessions-controller/protocols/_protocols/flow-internal/active-favicon.tssrc/main/controllers/tabs-controller/bounds.tssrc/main/controllers/tabs-controller/index.tssrc/main/controllers/tabs-controller/recently-closed-manager.tssrc/main/controllers/tabs-controller/recently-closed.tssrc/main/controllers/tabs-controller/tab-groups/glance.tssrc/main/controllers/tabs-controller/tab-groups/index.tssrc/main/controllers/tabs-controller/tab-groups/split.tssrc/main/controllers/tabs-controller/tab-layout.tssrc/main/controllers/tabs-controller/tab-lifecycle.tssrc/main/controllers/tabs-controller/tab-sync.tssrc/main/controllers/tabs-controller/tab.tssrc/main/controllers/windows-controller/layer-manager/index.tssrc/main/controllers/windows-controller/types/browser.tssrc/main/ipc/app/new-tab.tssrc/main/ipc/browser/find-in-page.tssrc/main/ipc/browser/history.tssrc/main/ipc/browser/navigation.tssrc/main/ipc/browser/pinned-tabs.tssrc/main/ipc/browser/prompts/page.tssrc/main/ipc/browser/tabs.tssrc/main/ipc/index.tssrc/main/ipc/webauthn/conditional.tssrc/main/modules/flags.tssrc/main/saving/db/schema.tssrc/main/saving/tabs/index.tssrc/main/saving/tabs/restore.tssrc/main/saving/tabs/serialization.tssrc/main/services/tab-service/AGENTS.mdsrc/main/services/tab-service/core/pinned-tab.tssrc/main/services/tab-service/core/recently-closed-manager.tssrc/main/services/tab-service/core/save-image-as.tssrc/main/services/tab-service/core/tab-context-menus.tssrc/main/services/tab-service/core/tab-layout-node.tssrc/main/services/tab-service/core/tab.tssrc/main/services/tab-service/core/web-context-menu.tssrc/main/services/tab-service/design.mdsrc/main/services/tab-service/index.tssrc/main/services/tab-service/ipc/preload-api.tssrc/main/services/tab-service/ipc/tab-ipc.tssrc/main/services/tab-service/layout/tab-layout.tssrc/main/services/tab-service/layout/tab-positioner.tssrc/main/services/tab-service/persistence/pinned-tab-persistence.tssrc/main/services/tab-service/persistence/tab-persistence-service.tssrc/main/services/tab-service/tab-lifecycle-timer.tssrc/main/services/tab-service/tab-service.tssrc/main/services/tab-service/tab-sync.tssrc/preload/index.tssrc/renderer/src/components/browser-ui/browser-content.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/_components/bottom/bottom-extras-menu.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/_components/bottom/space-switcher.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/_components/browser-action-list.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/normal/pin-grid.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/pinned-tab-button.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/_components/pin-grid/slot-machine/main.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/_components/site-controls/extensions.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/_components/space-pages-carousel.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/_components/tab-drop-target.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/_components/tab-group.tsxsrc/renderer/src/components/browser-ui/main.tsxsrc/renderer/src/components/browser-ui/target-url-indicator.tsxsrc/renderer/src/components/omnibox/main.tsxsrc/renderer/src/components/providers/pinned-tabs-provider.tsxsrc/renderer/src/components/providers/tab-service-provider.tsxsrc/renderer/src/components/providers/tabs-provider.tsxsrc/renderer/src/components/settings/sections/general/update-card.tsxsrc/renderer/src/lib/omnibox-new/suggestors/open-tabs.tssrc/renderer/src/lib/omnibox/data-providers/open-tabs.tssrc/renderer/src/lib/omnibox/omnibox.tssrc/renderer/src/routes/history/page.tsxsrc/shared/flow/flow.tssrc/shared/flow/interfaces/browser/pinned-tabs.tssrc/shared/flow/interfaces/browser/tab-service.tssrc/shared/flow/interfaces/browser/tabs.tssrc/shared/types/tab-service.ts
💤 Files with no reviewable changes (19)
- src/main/controllers/tabs-controller/tab-layout.ts
- src/main/saving/tabs/index.ts
- src/main/services/tab-service/core/save-image-as.ts
- src/main/ipc/browser/tabs.ts
- src/shared/flow/interfaces/browser/pinned-tabs.ts
- src/main/controllers/tabs-controller/tab-lifecycle.ts
- src/main/controllers/tabs-controller/tab-sync.ts
- src/main/saving/tabs/serialization.ts
- src/main/controllers/tabs-controller/bounds.ts
- src/main/controllers/tabs-controller/recently-closed-manager.ts
- src/main/controllers/tabs-controller/tab-groups/split.ts
- src/main/controllers/tabs-controller/tab-groups/glance.ts
- src/main/controllers/pinned-tabs-controller/index.ts
- src/main/controllers/tabs-controller/tab.ts
- src/shared/flow/interfaces/browser/tabs.ts
- src/main/ipc/browser/pinned-tabs.ts
- src/main/controllers/tabs-controller/tab-groups/index.ts
- src/main/controllers/tabs-controller/recently-closed.ts
- src/main/controllers/tabs-controller/index.ts
Comment on lines
+146
to
+147
| const targetWindow = window || browserWindowsController.getWindows()[0]; | ||
| if (!targetWindow) throw new Error("No window available"); |
Contributor
There was a problem hiding this comment.
Avoid cross-profile fallback when windowId is missing.
Falling back to the first global window can open a profile’s extension tab in a different profile window/session. Resolve a window scoped to profileId (or create one) before calling createTab.
Suggested fix
- const targetWindow = window || browserWindowsController.getWindows()[0];
+ const targetWindow =
+ window ?? browserWindowsController.getWindows().find((candidate) => candidate.profileId === profileId);
if (!targetWindow) throw new Error("No window available");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const targetWindow = window || browserWindowsController.getWindows()[0]; | |
| if (!targetWindow) throw new Error("No window available"); | |
| const targetWindow = | |
| window ?? browserWindowsController.getWindows().find((candidate) => candidate.profileId === profileId); | |
| if (!targetWindow) throw new Error("No window available"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/controllers/loaded-profiles-controller/index.ts` around lines 146 -
147, The code currently falls back to browserWindowsController.getWindows()[0]
when window is missing, which may open tabs in the wrong profile; instead
resolve or create a window scoped to the given profileId before calling
createTab: lookup an existing window for profileId via
browserWindowsController.getWindows().find(w => w.profileId === profileId) (or
use a browserWindowsController.getWindowForProfile(profileId) helper), and if
none exists create a new profile-scoped window before assigning targetWindow
(replace use of the global window || browserWindowsController.getWindows()[0]
with this profile-scoped resolution) so createTab is always invoked in the
correct profile context.
Comment on lines
+118
to
138
| function restoreLayoutNodes(persistedNodes: PersistedTabLayoutNodeData[], uniqueIdToTabId: Map<string, number>): void { | ||
| for (const nodeData of persistedNodes) { | ||
| const tabIds: number[] = []; | ||
| for (const uniqueId of groupData.tabUniqueIds) { | ||
| for (const uniqueId of nodeData.tabUniqueIds) { | ||
| const tabId = uniqueIdToTabId.get(uniqueId); | ||
| if (tabId !== undefined) { | ||
| tabIds.push(tabId); | ||
| } | ||
| } | ||
|
|
||
| if (tabIds.length < 2) { | ||
| // Tab groups need at least 2 tabs | ||
| try { | ||
| await tabPersistenceManager.removeTabGroup(groupData.groupId); | ||
| } catch (error) { | ||
| console.error("Failed to remove stale tab group:", error); | ||
| } | ||
| tabPersistenceService.removeLayoutNode(nodeData.id); | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| const group = tabsController.createTabGroup(groupData.mode, tabIds as [number, ...number[]], groupData.groupId); | ||
| // Get the window from the first tab | ||
| const firstTab = tabService.getTabById(tabIds[0]); | ||
| if (!firstTab) continue; | ||
|
|
||
| // Restore glance front tab | ||
| if (groupData.mode === "glance" && groupData.glanceFrontTabUniqueId) { | ||
| const frontTabId = uniqueIdToTabId.get(groupData.glanceFrontTabUniqueId); | ||
| if (frontTabId !== undefined && group instanceof GlanceTabGroup) { | ||
| group.setFrontTab(frontTabId); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error("Failed to restore tab group:", error); | ||
| } | ||
| tabService.createLayoutNode(firstTab.getWindow().id, nodeData.mode, tabIds); | ||
| } |
Contributor
There was a problem hiding this comment.
Restore frontTabUniqueId when recreating layout nodes.
restoreLayoutNodes maps tabUniqueIds but never restores frontTabUniqueId, so persisted front-tab state is lost on session restore.
Suggested direction
function restoreLayoutNodes(persistedNodes: PersistedTabLayoutNodeData[], uniqueIdToTabId: Map<string, number>): void {
for (const nodeData of persistedNodes) {
@@
- tabService.createLayoutNode(firstTab.getWindow().id, nodeData.mode, tabIds);
+ const frontTabId =
+ nodeData.frontTabUniqueId !== undefined ? uniqueIdToTabId.get(nodeData.frontTabUniqueId) : undefined;
+ tabService.createLayoutNode(firstTab.getWindow().id, nodeData.mode, tabIds, frontTabId);
}
}(If createLayoutNode doesn’t accept frontTabId yet, its signature should be extended so restore can preserve this field.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/saving/tabs/restore.ts` around lines 118 - 138, restoreLayoutNodes
currently translates nodeData.tabUniqueIds to tabIds but drops
nodeData.frontTabUniqueId, losing front-tab state; map nodeData.frontTabUniqueId
to a frontTabId via uniqueIdToTabId (similar to the tabIds loop) and pass that
frontTabId into tabService.createLayoutNode (or extend createLayoutNode's
signature to accept an optional frontTabId) so recreated layout nodes preserve
the front tab; update restoreLayoutNodes (and createLayoutNode
implementation/signature) to accept and use the restored frontTabId when
available.
Comment on lines
+13
to
+18
| ``` | ||
| tabService → TabService instance (central orchestrator) | ||
| tabPersistenceService → TabPersistenceService (save/restore to SQLite) | ||
| tabIPC → TabIPC (renderer communication) | ||
| initializeTabService() → called at app startup after DB is ready | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifiers to fenced code blocks
The unlabeled fenced blocks trigger markdownlint MD040 warnings.
Proposed fix
-```
+```text
tabService → TabService instance (central orchestrator)
tabPersistenceService → TabPersistenceService (save/restore to SQLite)
tabIPC → TabIPC (renderer communication)
initializeTabService() → called at app startup after DB is ready- +text
Tab property changes → Tab emits "updated" → wireTabEvents handler →
- tab.notifyExtensionsOfChanges() (emits "tab-updated" on webContents → extensions library)
- tabService.emitContentChange() (→ TabIPC debounced → renderer)
-```
+```text
tab-service/
index.ts Entry point, singleton creation, exports
tab-service.ts TabService class (~1800 lines)
tab-sync.ts Cross-window sync, placeholders
tab-lifecycle-timer.ts Auto-sleep/archive timer
core/
tab.ts Tab class (~850 lines)
tab-layout-node.ts TabLayoutNode class (~320 lines)
pinned-tab.ts PinnedTab class (~140 lines)
recently-closed-manager.ts Recently closed tab tracking
tab-context-menus.ts Sidebar context menu
web-context-menu.ts Page context menu
save-image-as.ts Save image dialog
layout/
tab-layout.ts TabLayout class (~380 lines)
tab-positioner.ts Position math (~70 lines)
ipc/
tab-ipc.ts IPC handlers + debounced emission (~570 lines)
preload-api.ts Preload bridge API types
persistence/
tab-persistence-service.ts Autosave/restore tabs
pinned-tab-persistence.ts Pinned tab DB operations
</details>
Also applies to: 51-55, 115-138
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @src/main/services/tab-service/AGENTS.md around lines 13 - 18, The markdown
file contains unlabeled fenced code blocks that trigger MD040; update each
unlabeled triple-backtick block in AGENTS.md to use a language identifier (use
"text") — specifically the blocks containing the short key/value list
(tabService → TabService ...), the "Tab property changes → Tab emits 'updated' →
wireTabEvents handler →" block, and the directory listing block starting with
"tab-service/" (and any other unlabeled fences such as those noted around 51-55
and 115-138) so they read text ... instead of plain ```; keep the block
contents unchanged.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+39
to
+45
| | Module | File | Purpose | | ||
| | ------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------- | | ||
| | `tab-sync.ts` | Tab sync across windows. Screenshot placeholders, `moveTabToWindowIfNeeded`, `ensureNodeInLayout`. Pinned tabs always sync; normal tabs sync when setting enabled. | | ||
| | `tab-lifecycle-timer.ts` | 10s interval that auto-sleeps/archives inactive tabs. | | ||
| | `tab-context-menus.ts` | Sidebar tab right-click menu (Copy URL, Mute, Duplicate, Move To, Close, Pin/Unpin, Reopen). | | ||
| | `web-context-menu.ts` | Web page right-click context menu. | | ||
| | `save-image-as.ts` | "Save Image As" dialog helper. | |
Contributor
There was a problem hiding this comment.
Fix malformed Supporting Modules table rows
The rows currently have fewer columns than the header, which breaks markdown table parsing.
Proposed fix
| Module | File | Purpose |
| ------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------- |
-| `tab-sync.ts` | Tab sync across windows. Screenshot placeholders, `moveTabToWindowIfNeeded`, `ensureNodeInLayout`. Pinned tabs always sync; normal tabs sync when setting enabled. |
-| `tab-lifecycle-timer.ts` | 10s interval that auto-sleeps/archives inactive tabs. |
-| `tab-context-menus.ts` | Sidebar tab right-click menu (Copy URL, Mute, Duplicate, Move To, Close, Pin/Unpin, Reopen). |
-| `web-context-menu.ts` | Web page right-click context menu. |
-| `save-image-as.ts` | "Save Image As" dialog helper. |
+| `tab-sync.ts` | `tab-sync.ts` | Tab sync across windows. Screenshot placeholders, `moveTabToWindowIfNeeded`, `ensureNodeInLayout`. Pinned tabs always sync; normal tabs sync when setting enabled. |
+| `tab-lifecycle-timer.ts` | `tab-lifecycle-timer.ts` | 10s interval that auto-sleeps/archives inactive tabs. |
+| `tab-context-menus.ts` | `core/tab-context-menus.ts` | Sidebar tab right-click menu (Copy URL, Mute, Duplicate, Move To, Close, Pin/Unpin, Reopen). |
+| `web-context-menu.ts` | `core/web-context-menu.ts` | Web page right-click context menu. |
+| `save-image-as.ts` | `core/save-image-as.ts` | "Save Image As" dialog helper. |🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 41-41: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
[warning] 42-42: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
[warning] 43-43: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
[warning] 44-44: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
[warning] 45-45: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/services/tab-service/AGENTS.md` around lines 39 - 45, The Supporting
Modules table rows in AGENTS.md are malformed because several rows only provide
two cells while the header requires three (Module | File | Purpose); update each
listed row (e.g., entries for tab-sync.ts, tab-lifecycle-timer.ts,
tab-context-menus.ts, web-context-menu.ts, save-image-as.ts) so they contain
three pipe-separated columns matching the header (Module, File, Purpose) and
ensure the separator row (| --- | --- | --- |) is present; keep the module name
(e.g., `tab-sync.ts`) in the first column, the file path/details in the second,
and the purpose text in the third for each row.
Comment on lines
+93
to
+100
| void tabService | ||
| .createTab(window.id, tab.profileId, targetSpaceId, undefined, { | ||
| url: tab.url, | ||
| position: tab.position + 0.5 | ||
| }) | ||
| .then(() => { | ||
| tabService.normalizePositions(window.id, targetSpaceId); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify this promise chain currently lacks rejection handling.
rg -nP 'void\\s+tabService\\s*\\.\\s*createTab\\([\\s\\S]*?\\)\\s*\\.then\\(' src/main/services/tab-service/core/tab-context-menus.ts
rg -nP '\\.catch\\(' src/main/services/tab-service/core/tab-context-menus.tsRepository: MultiboxLabs/flow-browser
Length of output: 215
Add rejection handling to duplicate-tab creation to avoid unhandled promise rejections (File: src/main/services/tab-service/core/tab-context-menus.ts, Lines: 93-100)
The duplicate-tab path chains .then(...) without any failure handling; if tabService.createTab(...) rejects, it can result in an unhandled promise rejection.
💡 Proposed fix
void tabService
.createTab(window.id, tab.profileId, targetSpaceId, undefined, {
url: tab.url,
position: tab.position + 0.5
})
.then(() => {
tabService.normalizePositions(window.id, targetSpaceId);
- });
+ })
+ .catch((error) => {
+ console.error("Failed to duplicate tab:", error);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void tabService | |
| .createTab(window.id, tab.profileId, targetSpaceId, undefined, { | |
| url: tab.url, | |
| position: tab.position + 0.5 | |
| }) | |
| .then(() => { | |
| tabService.normalizePositions(window.id, targetSpaceId); | |
| }); | |
| void tabService | |
| .createTab(window.id, tab.profileId, targetSpaceId, undefined, { | |
| url: tab.url, | |
| position: tab.position + 0.5 | |
| }) | |
| .then(() => { | |
| tabService.normalizePositions(window.id, targetSpaceId); | |
| }) | |
| .catch((error) => { | |
| console.error("Failed to duplicate tab:", error); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/services/tab-service/core/tab-context-menus.ts` around lines 93 -
100, The duplicate-tab creation call to tabService.createTab(...) currently only
attaches .then(...) and lacks rejection handling, risking unhandled promise
rejections; update the call site to handle failures by either awaiting the
promise inside an async function with try/catch or by chaining a .catch(...) to
tabService.createTab(window.id, tab.profileId, targetSpaceId, undefined, { url:
tab.url, position: tab.position + 0.5 }) and log or surface the error, ensuring
normalizePositions(window.id, targetSpaceId) runs only on success (or handle
recovery on failure) so failures are handled deterministically.
Comment on lines
+597
to
+621
| public createPinnedTabFromTab(tabId: number, position?: number): PinnedTab | null { | ||
| const tab = this.tabs.get(tabId); | ||
| if (!tab || !tab.url) return null; | ||
|
|
||
| const maxPos = this.getMaxPinnedTabPosition(tab.profileId); | ||
| const finalPosition = position ?? maxPos + 1; | ||
|
|
||
| const pinnedTab = PinnedTab.create(tab.profileId, tab.url, tab.faviconURL, finalPosition); | ||
|
|
||
| this.pinnedTabs.set(pinnedTab.uniqueId, pinnedTab); | ||
|
|
||
| // Mark the tab as owned by this pinned tab (ephemeral — remove stale DB record) | ||
| tab.owner = { kind: "pinned", pinnedTabId: pinnedTab.uniqueId }; | ||
| this.emitContentChange(tab.getWindow().id, tab.id); | ||
|
|
||
| // Associate the tab | ||
| pinnedTab.associate(tab.spaceId, tab.id); | ||
|
|
||
| this.wirePinnedTabEvents(pinnedTab); | ||
| this.normalizePinnedTabPositions(tab.profileId); | ||
| this.pinnedTabDb.save(pinnedTab); | ||
| this.emit("pinned-tab-changed"); | ||
| this.emitStructuralChange(tab.getWindow().id); | ||
|
|
||
| return pinnedTab; |
Contributor
There was a problem hiding this comment.
Propagate the existing node when pinning a live tab.
clickPinnedTab() sets pinnedTab.layoutNode and calls propagatePinnedTabNode(...), but createPinnedTabFromTab() never does. A freshly pinned existing tab therefore only exists in its current layout; the first click from another space/window misses targetLayout.getNodeForTab(...) and falls back to activating the old location instead of the requested one.
💡 Proposed fix
// Associate the tab
pinnedTab.associate(tab.spaceId, tab.id);
+ const layout = this.getLayout(tab.getWindow().id, tab.spaceId);
+ const node = layout?.getNodeForTab(tab.id);
+ if (node) {
+ pinnedTab.layoutNode = node;
+ this.propagatePinnedTabNode(node, pinnedTab.profileId);
+ }
this.wirePinnedTabEvents(pinnedTab);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public createPinnedTabFromTab(tabId: number, position?: number): PinnedTab | null { | |
| const tab = this.tabs.get(tabId); | |
| if (!tab || !tab.url) return null; | |
| const maxPos = this.getMaxPinnedTabPosition(tab.profileId); | |
| const finalPosition = position ?? maxPos + 1; | |
| const pinnedTab = PinnedTab.create(tab.profileId, tab.url, tab.faviconURL, finalPosition); | |
| this.pinnedTabs.set(pinnedTab.uniqueId, pinnedTab); | |
| // Mark the tab as owned by this pinned tab (ephemeral — remove stale DB record) | |
| tab.owner = { kind: "pinned", pinnedTabId: pinnedTab.uniqueId }; | |
| this.emitContentChange(tab.getWindow().id, tab.id); | |
| // Associate the tab | |
| pinnedTab.associate(tab.spaceId, tab.id); | |
| this.wirePinnedTabEvents(pinnedTab); | |
| this.normalizePinnedTabPositions(tab.profileId); | |
| this.pinnedTabDb.save(pinnedTab); | |
| this.emit("pinned-tab-changed"); | |
| this.emitStructuralChange(tab.getWindow().id); | |
| return pinnedTab; | |
| public createPinnedTabFromTab(tabId: number, position?: number): PinnedTab | null { | |
| const tab = this.tabs.get(tabId); | |
| if (!tab || !tab.url) return null; | |
| const maxPos = this.getMaxPinnedTabPosition(tab.profileId); | |
| const finalPosition = position ?? maxPos + 1; | |
| const pinnedTab = PinnedTab.create(tab.profileId, tab.url, tab.faviconURL, finalPosition); | |
| this.pinnedTabs.set(pinnedTab.uniqueId, pinnedTab); | |
| // Mark the tab as owned by this pinned tab (ephemeral — remove stale DB record) | |
| tab.owner = { kind: "pinned", pinnedTabId: pinnedTab.uniqueId }; | |
| this.emitContentChange(tab.getWindow().id, tab.id); | |
| // Associate the tab | |
| pinnedTab.associate(tab.spaceId, tab.id); | |
| const layout = this.getLayout(tab.getWindow().id, tab.spaceId); | |
| const node = layout?.getNodeForTab(tab.id); | |
| if (node) { | |
| pinnedTab.layoutNode = node; | |
| this.propagatePinnedTabNode(node, pinnedTab.profileId); | |
| } | |
| this.wirePinnedTabEvents(pinnedTab); | |
| this.normalizePinnedTabPositions(tab.profileId); | |
| this.pinnedTabDb.save(pinnedTab); | |
| this.emit("pinned-tab-changed"); | |
| this.emitStructuralChange(tab.getWindow().id); | |
| return pinnedTab; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/services/tab-service/tab-service.ts` around lines 597 - 621,
createPinnedTabFromTab currently creates a PinnedTab but never propagates the
existing layout node, so clicks later rely on stale locations; update
createPinnedTabFromTab to copy the live tab's layout node into
pinnedTab.layoutNode (from the source Tab) and call
propagatePinnedTabNode(pinnedTab, /* appropriate args if required */) the same
way clickPinnedTab does, ensuring the new pinned tab is registered in the layout
before saving/emit events (refer to createPinnedTabFromTab, clickPinnedTab,
pinnedTab.layoutNode, and propagatePinnedTabNode to locate the spots to modify).
Comment on lines
+904
to
+960
| public moveTabToSpace(tabId: number, spaceId: string, newPosition?: number): void { | ||
| const tab = this.tabs.get(tabId); | ||
| if (!tab) return; | ||
|
|
||
| const sourceSpaceId = tab.spaceId; | ||
| if (sourceSpaceId === spaceId) return; | ||
|
|
||
| const windowId = tab.getWindow().id; | ||
| const sourceLayout = this.getLayout(windowId, sourceSpaceId); | ||
| const targetLayout = this.getOrCreateLayout(windowId, spaceId); | ||
|
|
||
| // Hide the tab before moving (it's leaving the source space) | ||
| if (tab.visible) { | ||
| tab.visible = false; | ||
| tab.layer?.setVisible(false); | ||
| } | ||
|
|
||
| // Remove from source layout | ||
| if (sourceLayout) { | ||
| const node = sourceLayout.getNodeForTab(tab.id); | ||
| if (node) { | ||
| // Destroy single node from source, or remove tab from multi-node. | ||
| // The "destroyed" event cascades cleanup to all member layouts. | ||
| if (node.mode === "single") { | ||
| sourceLayout.destroyNode(node.id); | ||
| } else { | ||
| node.removeTab(tab); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Update tab's space | ||
| tab.setSpace(spaceId); | ||
|
|
||
| // Create a new node in the target layout | ||
| targetLayout.createSingleNode(tab); | ||
|
|
||
| // Clear focused tab references to this tab in the source space across ALL layouts. | ||
| for (const layout of this.layouts.values()) { | ||
| if (layout.spaceId === sourceSpaceId && layout.getFocusedTab()?.id === tab.id) { | ||
| layout.removeFocusedTab(); | ||
| } | ||
| } | ||
|
|
||
| if (newPosition !== undefined) { | ||
| tab.updateStateProperty("position", newPosition); | ||
| } | ||
|
|
||
| // Normalize both spaces | ||
| this.positioner.normalizePositions(this.getTabsInWindowSpace(windowId, spaceId)); | ||
| this.positioner.normalizePositions(this.getTabsInWindowSpace(windowId, sourceSpaceId)); | ||
|
|
||
| // Notify extensions that indices shifted (tab moved between spaces) | ||
| this.notifyIndexChanges(windowId, tab.profileId); | ||
|
|
||
| // Notify renderer that source space changed (tab removed) | ||
| this.emitStructuralChange(windowId); |
Contributor
There was a problem hiding this comment.
Select a replacement active node when moving the active tab out of the current space.
This removes or destroys the source node, but never asks sourceLayout to pick the next tab. If the moved tab was the active tab in the currently visible space, activeNode becomes null and the window is left blank until some later action activates another node.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/services/tab-service/tab-service.ts` around lines 904 - 960, When
removing/destroying the source node in moveTabToSpace, ensure the source layout
selects a replacement active node if the moved tab was the currently focused
tab: after the removal/destroy logic in moveTabToSpace, check if
sourceLayout.getFocusedTab()?.id === tab.id and if so call a layout API to
pick/activate a replacement (e.g. sourceLayout.selectReplacementActiveNode() or
sourceLayout.focusNextAvailableTab()); alternatively call
sourceLayout.removeFocusedTab() immediately for that specific sourceLayout and
then invoke its replacement-selection routine so the visible window isn’t left
blank. Ensure you use the existing symbols moveTabToSpace, sourceLayout,
getNodeForTab, getFocusedTab and removeFocusedTab to locate where to add this
behavior.
Comment on lines
+1021
to
+1025
| // Forward events | ||
| layout.on("active-changed", (wId, sId) => { | ||
| this.updateTabVisibility(wId, sId); | ||
| this.emit("active-changed", wId, sId); | ||
| }); |
Contributor
There was a problem hiding this comment.
Don't reshow tabs when a hidden layout changes active node.
The active-changed listener always calls updateTabVisibility(), and updateTabVisibility() ignores layout.visible. That means activateTab() on a non-current space—or pinned/STAW activation in another layout—can make a hidden space's tabs visible immediately, even though the comment in activateTab() says those activations should stay invisible until the user switches spaces.
💡 Proposed fix
layout.on("active-changed", (wId, sId) => {
- this.updateTabVisibility(wId, sId);
+ if (layout.visible) {
+ this.updateTabVisibility(wId, sId);
+ }
this.emit("active-changed", wId, sId);
});Also applies to: 1096-1141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/services/tab-service/tab-service.ts` around lines 1021 - 1025, The
active-changed listener currently always calls updateTabVisibility(wId, sId)
which ignores layout.visibility and causes tabs in hidden layouts to be
force-shown; fix by making the visibility check explicit: either have the
layout.on("active-changed", ...) handler only call updateTabVisibility when
layout.visible is true, or modify updateTabVisibility to early-return when the
originating layout (or its visible flag) is false so activateTab() and
pinned/STAW activations in non-current spaces do not reveal tabs until the user
actually switches spaces; apply the same guard to the other active-changed
handlers (the similar block around the activateTab/pinned logic) so hidden
layouts never trigger UI updates.
Comment on lines
+1493
to
+1533
| private handleNewTabRequested( | ||
| sourceTab: Tab, | ||
| url: string, | ||
| disposition: "new-window" | "foreground-tab" | "background-tab" | "default" | "other", | ||
| constructorOptions: Electron.WebContentsViewConstructorOptions | undefined, | ||
| handlerDetails: Electron.HandlerDetails | undefined, | ||
| options: { noLoadURL?: boolean } | ||
| ): void { | ||
| let windowId = sourceTab.getWindow().id; | ||
|
|
||
| if (disposition === "new-window") { | ||
| const parsedFeatures: Record<string, string | number> = {}; | ||
| if (handlerDetails?.features) { | ||
| for (const feature of handlerDetails.features.split(",")) { | ||
| const [key, value] = feature.trim().split("="); | ||
| if (key && value) { | ||
| parsedFeatures[key] = Number.isNaN(+value) ? value : +value; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const popupWindow = browserWindowsController.instantCreate("popup", { | ||
| ...(parsedFeatures.width ? { width: +parsedFeatures.width } : {}), | ||
| ...(parsedFeatures.height ? { height: +parsedFeatures.height } : {}), | ||
| ...(parsedFeatures.left ? { x: +parsedFeatures.left } : {}), | ||
| ...(parsedFeatures.top ? { y: +parsedFeatures.top } : {}) | ||
| }); | ||
| windowId = popupWindow.id; | ||
| popupWindow.setCurrentSpace(sourceTab.spaceId); | ||
| } | ||
|
|
||
| const insertPosition = disposition !== "new-window" ? sourceTab.position + 0.5 : undefined; | ||
|
|
||
| const isBackground = disposition === "background-tab"; | ||
| const newTab = this.createTabInternal(windowId, sourceTab.profileId, sourceTab.spaceId, undefined, { | ||
| url, | ||
| noLoadURL: options.noLoadURL, | ||
| webContentsViewOptions: constructorOptions, | ||
| position: insertPosition, | ||
| makeActive: !isBackground | ||
| }); |
Contributor
There was a problem hiding this comment.
Open renderer-initiated tabs in the window's active space, not the pinned tab's creation space.
handleNewTabRequested() always uses sourceTab.spaceId. For pinned/STAW tabs, that can be a stale creation space while the view is currently rendered in another window/space, so window.open / foreground-tab / background-tab requests can be created off-screen in the wrong layout.
💡 Proposed fix
- const newTab = this.createTabInternal(windowId, sourceTab.profileId, sourceTab.spaceId, undefined, {
+ const targetWindow = browserWindowsController.getWindowById(windowId);
+ const targetSpaceId = targetWindow?.currentSpaceId ?? sourceTab.spaceId;
+ const newTab = this.createTabInternal(windowId, sourceTab.profileId, targetSpaceId, undefined, {
url,
noLoadURL: options.noLoadURL,
webContentsViewOptions: constructorOptions,
position: insertPosition,
makeActive: !isBackground🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/services/tab-service/tab-service.ts` around lines 1493 - 1533,
handleNewTabRequested currently always uses sourceTab.spaceId when creating the
new tab, which can be stale for pinned/STAW tabs; instead query the target
window's active space and use that for the new tab's spaceId. Update the
function to get the target window object for windowId (e.g., via
browserWindowsController.get(windowId) or similar API), read its active/current
space id (e.g., targetWindow.getCurrentSpace() or targetWindow.activeSpaceId),
and pass that space id into createTabInternal (falling back to sourceTab.spaceId
if no active space is available); also ensure when creating a popup you still
call popupWindow.setCurrentSpace(...) using the resolved active space where
appropriate.
Comment on lines
126
to
129
| (e: React.MouseEvent) => { | ||
| e.preventDefault(); | ||
| flow.tabs.showContextMenu(tab.id); | ||
| flow.tabService.showContextMenu(tab.id); | ||
| }, |
Contributor
There was a problem hiding this comment.
Guard context-menu calls when tab.id is missing.
This handler can pass an invalid tab id, while adjacent handlers already guard for missing ids.
Suggested fix
const handleContextMenu = useCallback(
(e: React.MouseEvent) => {
e.preventDefault();
+ if (!tab.id) return;
flow.tabService.showContextMenu(tab.id);
},
[tab.id]
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| (e: React.MouseEvent) => { | |
| e.preventDefault(); | |
| flow.tabs.showContextMenu(tab.id); | |
| flow.tabService.showContextMenu(tab.id); | |
| }, | |
| (e: React.MouseEvent) => { | |
| e.preventDefault(); | |
| if (!tab.id) return; | |
| flow.tabService.showContextMenu(tab.id); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/renderer/src/components/browser-ui/browser-sidebar/_components/tab-group.tsx`
around lines 126 - 129, The context-menu click handler calls
flow.tabService.showContextMenu(tab.id) without verifying tab.id; add a guard in
that handler (the anonymous React.MouseEvent handler around
flow.tabService.showContextMenu) to return early if tab.id is null/undefined (or
otherwise invalid) before invoking flow.tabService.showContextMenu, mirroring
adjacent handlers that check the id so the service never receives an invalid id.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ider - Remove unused TabServiceProvider (duplicate IPC subscriptions) - Rename TabGroup → TabLayoutNodeView / TabLayoutNode across provider and sidebar - Rename drag payload type tab-group → tab-layout-node - Fix stale PersistedTabData comment in shared/types/tabs.ts
Comment on lines
+276
to
+277
| if (this.view && this.layer) { | ||
| window.layerManager?.push(this.layer); |
Contributor
Author
There was a problem hiding this comment.
🔴 Stale LayerManager reference in Layer after cross-window tab move
When setWindow moves a tab to a new window, it pops the existing Layer from the old window's LayerManager and pushes it directly to the new window's LayerManager without recreating the Layer object. The Layer retains its internal this.manager reference pointing to the old window's LayerManager. This causes layer.focus() (at src/main/controllers/windows-controller/layer-manager/index.ts:52) to call this.manager.clearPendingFocusReallocation() on the wrong manager, and setVisible(false) to trigger reallocateFocus() on the wrong manager. The old code in the deleted tab.ts:750-776 always recreated the Layer with new Layer(window.layerManager, ...) when the window changed. This affects all cross-window tab moves (STAW focus-move, pinned tab clicks across windows, tab relocation from closing windows).
Prompt for agents
In Tab.setWindow() (core/tab.ts:264-302), when a Layer already exists and the tab moves to a new window, the Layer object is reused without updating its internal `manager` reference. The old code (deleted tab.ts) recreated the Layer: `this.layer = new Layer(window.layerManager, this.view, zIndexes[layerType], focusPriorities[layerType], createModalTo(layerType))`. The fix should recreate the Layer for the new window's LayerManager when the window changes, similar to how `setLayerType` (tab.ts:308-319) already does it correctly. Specifically, in the `if (this.view && this.layer)` branch, instead of just pushing the old layer, pop it from the old manager, create a new Layer with `window.layerManager`, and push that. Preserve the layer's visibility state across the recreation.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+258
to
+265
| const tab = await this.tabService.createTab(window.id, space.profileId, spaceId, undefined, { | ||
| url: url || undefined, | ||
| typedNavigation: typedFromAddressBar === true | ||
| }); | ||
|
|
||
| if (isForeground) { | ||
| this.tabService.activateTab(tab); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
🟡 IPC newTab handler always activates tab regardless of isForeground parameter
createTab() calls createTabInternal() which auto-activates the new tab by default (options.makeActive !== false at tab-service.ts:240). The IPC handler at tab-ipc.ts:258 calls createTab() without passing makeActive: false when isForeground is false, so every tab created through the tab-service:new-tab IPC channel is immediately activated. The if (isForeground) { this.tabService.activateTab(tab) } check on line 263 becomes redundant. The old handler (ipc/browser/tabs.ts:282-291) correctly relied on createTab NOT auto-activating. This breaks background tab creation — e.g., the slot machine Easter egg creates tabs with flow.tabService.newTab(url, false) expecting them to stay in the background, but they all get activated sequentially.
Suggested change
| const tab = await this.tabService.createTab(window.id, space.profileId, spaceId, undefined, { | |
| url: url || undefined, | |
| typedNavigation: typedFromAddressBar === true | |
| }); | |
| if (isForeground) { | |
| this.tabService.activateTab(tab); | |
| } | |
| const tab = await this.tabService.createTab(window.id, space.profileId, spaceId, undefined, { | |
| url: url || undefined, | |
| typedNavigation: typedFromAddressBar === true, | |
| makeActive: isForeground !== false | |
| }); | |
| if (isForeground) { | |
| this.tabService.activateTab(tab); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+54
to
56
| tabService.on("focused-tab-changed", (windowId, spaceId) => { | ||
| this.syncFocusedWindowHandoffActivity(windowId, spaceId, "active-tab-changed"); | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
🟡 Handoff activity not updated on space switch (macOS regression)
The old HandoffController listened to tabsController.on("current-space-changed") to update the macOS Handoff URL when the user switched spaces. The new code replaces this with tabService.on("focused-tab-changed") (src/main/controllers/handoff-controller/index.ts:54), but setCurrentWindowSpace (tab-service.ts:1233) does NOT emit focused-tab-changed when switching to a space that already has an established focused tab. The emitStructuralChange and updateTabVisibility calls in setCurrentWindowSpace don't produce a focused-tab-changed or active-changed event if the new space's layout already had its active node set from a previous visit. This means the macOS Handoff activity URL becomes stale when switching between spaces.
Prompt for agents
The HandoffController at src/main/controllers/handoff-controller/index.ts:54-56 listens only to "focused-tab-changed" but TabService.setCurrentWindowSpace (tab-service.ts:1233) does not emit this event when switching to a space that already has a focused tab established. The fix should either: (1) Add a "current-space-changed" event to TabServiceEvents and emit it from setCurrentWindowSpace, then listen for it in HandoffController, or (2) Emit "focused-tab-changed" from setCurrentWindowSpace when the visible layout changes and has a focused tab. Option 1 is simpler and matches the old architecture more closely.
Was this helpful? React with 👍 or 👎 to provide feedback.
iamEvanYT
added
the
enhancement
Devin.ai Integration
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Complete OOP-based Tab Service v2 replacement for the old tabs-controller. This PR:
window.open()/ web context-menu new tabs resolve the active target window/space at action time.Architecture
TabService— central orchestrator (tabs, pinned tabs, layouts, positioner)Tab— OOP entity with lifecycle (create → sleep → wake → destroy)PinnedTab— persistent entity with per-space associationsTabLayout/TabLayoutNode— per-window layout managementTabIPC— batched structural/content IPC with sync-aware broadcastingTabSync— handles tab view relocation between windows (focus-move, placeholder screenshots)TabPersistenceService— dirty-tracking + periodic flush to SQLiteTabOwnerRefdiscriminated union (normal | pinned | bookmark) for future-proofingReview & Testing Checklist for Human
window.open()for foreground/background/popup cases → new tabs open in the active window/space, not the tab's original creation spaceNotes
bun run format,bun run typecheck, andbun run lint.build (ubuntu-24.04-arm)CI check may fail due to a known infrastructure issue withapp-builder-bin(preexisting, not caused by this PR)capturePage()which requires the tab to have been rendered at least onceLink to Devin session: https://app.devin.ai/sessions/24bce8b710ba4e62b93215436e0b26a9
Requested by: @iamEvanYT