Conversation
…tab moves The tabs:move-tab-to-window-space IPC handler previously normalized positions in the source space using the target window ID. For cross-window moves, the source space lives in a different window, so we now capture sourceWindowId before the move and use it for the source normalization.
Use pragmatic-dnd's external adapter to handle drags arriving from other Electron windows via the native OS drag system: - tab-group.tsx: add getInitialDataForExternal on draggable() to serialize TabGroupSourceData to a custom MIME type, add dropTargetForExternal to accept external drops, refactor drop logic into shared handleDrop. - tab-drop-target.tsx: add dropTargetForExternal alongside existing dropTargetForElements, refactor into shared handleDrop. - space-switcher.tsx: add dropTargetForExternal to SpaceButton so tabs from other windows can be dropped onto space icons. All registrations use combine() for cleanup. canDrop for external drops checks MIME type presence only (HTML5 DnD security restriction); profile compatibility is verified in onDrop when data becomes available.
Mirror the new browser UI cross-window DnD changes for the old UI:
- sidebar-tab-groups.tsx: add getInitialDataForExternal, dropTargetForExternal,
shared handleDrop, and combine() cleanup.
- sidebar-tab-drop-target.tsx: add dropTargetForExternal with shared handleDrop.
- spaces-switcher.tsx: add dropTargetForExternal to SpaceButton.
Also removes a stray console.log('clicked') from spaces-switcher.tsx.
Build artifacts for all platforms are ready! 🚀Download the artifacts for: One-line installer (Unstable):bunx flow-debug-build --open 22693835762(execution 22693835762 / attempt 1) |
Greptile SummaryThis PR adds cross-window tab drag-and-drop to Flow Browser by layering Remaining concerns:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant RendererA as Source Window (Renderer A)
participant MainProc as Main Process
participant OS as OS Drag System
participant RendererB as Drop Window (Renderer B)
RendererA->>MainProc: drag:register-token(token, tabId) [ipcRenderer.send]
Note over MainProc: activeToken = { token, tabId }
RendererA->>OS: getInitialDataForExternal()<br/>DataTransfer: { payload+token, profile-MIME }
OS-->>RendererB: dragenter / drag events<br/>(MIME type names visible, values hidden)
RendererB->>RendererB: canDrop: canDropExternalTabGroup()<br/>checks TAB_GROUP_MIME_TYPE +<br/>profile-specific MIME name
OS-->>RendererB: drop event<br/>(MIME values now readable)
RendererB->>RendererB: parseExternalTabGroupDrop()<br/>parse JSON, verify token present
RendererB->>MainProc: tabs:move-tab-to-window-space(tabId, spaceId, pos, token) [ipcRenderer.invoke]
MainProc->>MainProc: validateAndConsumeToken(token, tabId)<br/>clears activeToken on success
MainProc->>MainProc: tab.setSpace(spaceId)<br/>tab.setWindow(targetWindow)<br/>normalizePositions(source + target)
MainProc-->>RendererA: tabs:on-data-changed (structural refresh)
MainProc-->>RendererB: tabs:on-data-changed (structural refresh)
Last reviewed commit: d161adf |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/renderer/src/components/browser-ui/browser-sidebar/_components/space-switcher.tsx
Outdated
Show resolved
Hide resolved
src/renderer/src/components/old-browser-ui/sidebar/spaces-switcher.tsx
Outdated
Show resolved
Hide resolved
For external drops, canDrop can only check MIME type presence (HTML5 DnD security prevents reading data until drop). The shared handleDrop was missing profile compatibility and same-space guards that internal drops got from canDrop. Without these, an external drop from a different profile or same space would incorrectly proceed to moveTabToWindowSpace.
|
@greptile review |
- Add isExternal flag to drop handlers so cross-window drops to the same
space correctly call moveTabToWindowSpace instead of just reordering
- Encode profileId into a MIME type (application/x-flow-tab-group-profile-{id})
so external canDrop can reject incompatible profiles during drag, preventing
drop indicators and space switches for tabs from different profiles
- Restore space-switcher hover-to-switch behavior for same-profile cross-window
drags now that profile can be checked via MIME type
|
@greptile review |
src/renderer/src/components/browser-ui/browser-sidebar/_components/space-switcher.tsx
Outdated
Show resolved
Hide resolved
src/renderer/src/components/old-browser-ui/sidebar/spaces-switcher.tsx
Outdated
Show resolved
Hide resolved
src/renderer/src/components/browser-ui/browser-sidebar/_components/tab-group.tsx
Outdated
Show resolved
Hide resolved
…ed MIME constants - Add isExternal parameter to handleDrop in both space-switcher files so same-space cross-window drops call moveTabToWindowSpace instead of being silently discarded - Extract TAB_GROUP_MIME_TYPE and TAB_GROUP_PROFILE_MIME_PREFIX to a shared module (lib/tab-drag-mime.ts) to eliminate duplication across 6 files
|
@greptile review |
src/renderer/src/components/browser-ui/browser-sidebar/_components/tab-group.tsx
Outdated
Show resolved
Hide resolved
src/renderer/src/components/browser-ui/browser-sidebar/_components/tab-group.tsx
Show resolved
Hide resolved
|
@greptile review |
| export function registerToken(token: string, tabId: number): void { | ||
| activeToken = { token, tabId }; | ||
| } |
There was a problem hiding this comment.
Stale token persists after a cancelled drag
When a drag is initiated, registerToken is called immediately. If the user cancels the drag (presses Escape, releases outside a valid target, etc.), the token is never consumed and activeToken keeps the stale entry indefinitely. The stale token is only evicted if the user starts a new drag (which replaces activeToken) or completes a valid drop.
In practice the stale token cannot be replayed by the renderer — getInitialDataForExternal is a fire-and-forget closure and never stores the token in component state, so the renderer has no reference to it after the callback returns. The risk is low, but consider adding a cleanup on dragend or a short TTL to make the guarantee explicit:
// Option: auto-expire the token after a safe window (e.g. 30 s)
let expiryTimeout: ReturnType<typeof setTimeout> | null = null;
export function registerToken(token: string, tabId: number): void {
if (expiryTimeout) clearTimeout(expiryTimeout);
activeToken = { token, tabId };
expiryTimeout = setTimeout(() => { activeToken = null; }, 30_000);
}
export function validateAndConsumeToken(token: string, tabId: number): boolean {
if (!activeToken) return false;
if (activeToken.token !== token || activeToken.tabId !== tabId) return false;
if (expiryTimeout) { clearTimeout(expiryTimeout); expiryTimeout = null; }
activeToken = null;
return true;
}| ipcMain.on("drag:register-token", (_event, token: string, tabId: number) => { | ||
| registerToken(token, tabId); | ||
| }); |
There was a problem hiding this comment.
drag:register-token does not verify tab ownership
The handler accepts any (token, tabId) pair without checking that tabId is a tab actually owned by the sending window. Any renderer context holding "browser" permission (i.e. any trusted browser-UI window) can pre-register a token for a tab in a different window, then immediately call moveTabToWindowSpace with that token to move the tab without a real drag event.
This is currently low-risk because all browser-UI windows are equally trusted, but tightening the check makes the security model explicit:
ipcMain.on("drag:register-token", (event, token: string, tabId: number) => {
const webContents = event.sender;
const senderWindow = browserWindowsController.getWindowFromWebContents(webContents);
if (!senderWindow) return;
// Ensure the tab being registered belongs to the sender's window
const tab = tabsController.getTabById(tabId);
if (!tab || tab.getWindow()?.id !== senderWindow.id) return;
registerToken(token, tabId);
});| onDrop: (args) => { | ||
| stopDragging(); | ||
|
|
||
| const sourceData = parseExternalTabGroupDrop(args.source); | ||
| if (!sourceData) return; | ||
| handleDrop(sourceData, true); | ||
| } |
There was a problem hiding this comment.
stopDragging() called twice for external drops
stopDragging() is called on line 117 before handleDrop is invoked, and then again as the very first statement inside handleDrop (line 75). The double-call is harmless (idempotent), but it's inconsistent with the internal onDrop path (which calls handleDrop directly without a preceding stopDragging()). Removing the redundant call makes the two paths symmetrical:
| onDrop: (args) => { | |
| stopDragging(); | |
| const sourceData = parseExternalTabGroupDrop(args.source); | |
| if (!sourceData) return; | |
| handleDrop(sourceData, true); | |
| } | |
| onDrop: (args) => { | |
| const sourceData = parseExternalTabGroupDrop(args.source); | |
| if (!sourceData) return; | |
| handleDrop(sourceData, true); | |
| } |
The same pattern occurs in src/renderer/src/components/old-browser-ui/sidebar/spaces-switcher.tsx at lines 111–117.
| flow.tabs.moveTabToWindowSpace(sourceTabId, spaceData.id, newPos, sourceData.dragToken); | ||
| } |
There was a problem hiding this comment.
Passes drag credential unconditionally; inconsistent with tab-group.tsx
handleDrop always passes the drag credential from sourceData to moveTabToWindowSpace, for both internal and external drops. This is safe today because getInitialData (used for internal drags) does not populate the credential field, leaving it as undefined, and the IPC handler skips validation when no credential is supplied.
tab-group.tsx takes the more explicit approach, passing undefined for internal drops and only forwarding the credential when isExternal is true. Aligning this file with that pattern makes the design intent self-documenting and prevents a future regression if internal drag payloads ever accidentally gain a credential field.
The same concern applies to src/renderer/src/components/old-browser-ui/sidebar/content/sidebar-tab-drop-target.tsx at line 50.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
iamEvanYT
force-pushed
the
main
branch
2 times, most recently
from
ce3f35d to
b2cbe93
Compare
1 participant
Summary
Enables dragging tabs between different Flow browser windows using
@atlaskit/pragmatic-drag-and-drop's external/native drag adapter.tabs:move-tab-to-window-spacehandler now captures the source window ID before the move so position normalization targets the correct window during cross-window transfers.getInitialDataForExternalondraggable()to serialize tab data to a custom MIME type (application/x-flow-tab-group), anddropTargetForExternalon tab groups, the empty-area drop target, and space switcher buttons.isExternalflag so that cross-window drops to the same space correctly callmoveTabToWindowSpaceinstead of just reordering locally.application/x-flow-tab-group-profile-{id}) is registered alongside the payload during drag. ExternalcanDropchecks for this type to reject incompatible profiles during drag — preventing drop indicators, space switches, and drops for tabs from different profiles.How it works
getInitialDataForExternalwritesTabGroupSourceDataJSON to the nativedataTransferusing a custom MIME type — this data crosses Electron window boundaries via the OS drag system.dropTargetForExternalon each drop target listens for drags arriving from outside the current window's JS context.canDropchecks for the profile-specific MIME type, blocking indicators and interactions for incompatible profiles.isExternalflag to ensure cross-window drops always callmoveTabToWindowSpace(even for same-space moves between windows).moveTabToWindowSpaceIPC call handles the actual cross-window move since tab IDs are globally stable.Changed files
src/main/ipc/browser/tabs.tsbrowser-ui/tab-group.tsxisExternalflag in drop handlerbrowser-ui/tab-drop-target.tsxisExternalflagbrowser-ui/space-switcher.tsxold-browser-ui/sidebar-tab-groups.tsxold-browser-ui/sidebar-tab-drop-target.tsxold-browser-ui/spaces-switcher.tsx