Skip to content

Rearrange document tabs via drag and drop#3707

Closed
TrueDoctor wants to merge 1 commit intomasterfrom
document-tab-rearranging
Closed

Rearrange document tabs via drag and drop#3707
TrueDoctor wants to merge 1 commit intomasterfrom
document-tab-rearranging

Conversation

@TrueDoctor
Copy link
Copy Markdown
Member

Allows rearranging document tabs in the graphite ui using drag and drop.
Note this code has been ai generated. It has been tested to work, but is more intended as a proof of concept and potential blueprint for a proper pr.

@TrueDoctor TrueDoctor requested a review from Keavon February 2, 2026 12:07
@github-actions github-actions bot temporarily deployed to graphite-dev (Preview) February 2, 2026 12:11 Inactive
@Keavon Keavon force-pushed the document-tab-rearranging branch from ac697ea to 2d1fa0a Compare February 24, 2026 11:04
@TrueDoctor
Copy link
Copy Markdown
Member Author

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces drag-and-drop functionality for reordering document tabs, a great enhancement for user experience. The implementation spans both the Rust backend and the Svelte frontend, with a new ReorderDocument message and corresponding handler. The core drag-and-drop logic resides in the Panel.svelte component. While the feature is functional, I've identified a key area for improvement in the event listener management within Panel.svelte to boost performance and prevent potential conflicts. My review comment details this suggestion to help refine this proof-of-concept into a production-ready feature.

Comment on lines +179 to +191
onMount(() => {
addEventListener("pointermove", draggingPointerMove);
addEventListener("pointerup", draggingPointerUp);
addEventListener("mousedown", draggingMouseDown);
addEventListener("keydown", draggingKeyDown);
});

onDestroy(() => {
removeEventListener("pointermove", draggingPointerMove);
removeEventListener("pointerup", draggingPointerUp);
removeEventListener("mousedown", draggingMouseDown);
removeEventListener("keydown", draggingKeyDown);
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Global event listeners for drag-and-drop are added in onMount and removed in onDestroy. This is inefficient as they are active throughout the component's lifecycle, even when no drag is occurring. This can lead to performance degradation and potential conflicts with other parts of the application.

A more performant and robust approach is to attach these listeners to the window object only when a drag operation begins (i.e., in tabPointerDown) and remove them once it's complete (in a cleanup function called from draggingPointerUp and other abort scenarios).

To implement this, you can:

  1. Remove the onMount and onDestroy lifecycle hooks for these listeners.
  2. Add window.addEventListener(...) for pointermove, pointerup, mousedown, and keydown inside tabPointerDown.
  3. Add window.removeEventListener(...) for the same events inside abortDrag.

@github-actions github-actions bot temporarily deployed to graphite-dev (Preview) February 24, 2026 11:16 Inactive
@TrueDoctor
Copy link
Copy Markdown
Member Author

@cubic-dev-ai

@cubic-dev-ai
Copy link
Copy Markdown
Contributor

cubic-dev-ai bot commented Mar 8, 2026

@cubic-dev-ai

@TrueDoctor I have started the AI code review. It will take a few minutes to complete.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 5 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="frontend/src/components/window/Panel.svelte">

<violation number="1" location="frontend/src/components/window/Panel.svelte:142">
P2: Accessing `dragState.startIndex` without a null check. `dragState` is typed `DragState | undefined` and the enclosing `if` condition doesn't narrow it. Add an explicit guard to avoid a potential runtime `TypeError`.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.


function draggingPointerUp() {
if (dragInPanel && dragDropIndex !== undefined) {
reorderAction?.(dragState.startIndex, dragDropIndex);
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Accessing dragState.startIndex without a null check. dragState is typed DragState | undefined and the enclosing if condition doesn't narrow it. Add an explicit guard to avoid a potential runtime TypeError.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/window/Panel.svelte, line 142:

<comment>Accessing `dragState.startIndex` without a null check. `dragState` is typed `DragState | undefined` and the enclosing `if` condition doesn't narrow it. Add an explicit guard to avoid a potential runtime `TypeError`.</comment>

<file context>
@@ -58,19 +74,139 @@
+
+	function draggingPointerUp() {
+		if (dragInPanel && dragDropIndex !== undefined) {
+			reorderAction?.(dragState.startIndex, dragDropIndex);
+			justFinishedDrag = true;
+			// Clear after the current tick so a same-tick click is still suppressed, but the next intentional click is not swallowed
</file context>
Suggested change
reorderAction?.(dragState.startIndex, dragDropIndex);
if (dragState) reorderAction?.(dragState.startIndex, dragDropIndex);
Fix with Cubic

@Keavon Keavon force-pushed the master branch 6 times, most recently from d6228da to e58c1de Compare March 16, 2026 23:03
@Keavon Keavon force-pushed the master branch 5 times, most recently from 9b97ab7 to 2e842cb Compare March 19, 2026 11:00
@Keavon
Copy link
Copy Markdown
Member

Keavon commented Apr 3, 2026

Superseded by #3999.

@Keavon Keavon closed this Apr 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants