Conversation
There was a problem hiding this comment.
Code Review
This pull request implements document tab reordering via drag-and-drop. The changes include a new ReorderDocument message and handler in the Rust backend to update the document sequence, frontend logic in Panel.svelte to manage drag-and-drop state and pointer events, and persistence logic to save the tab order. Feedback is provided regarding a bug in the insertion marker's positioning logic where scrollLeft is incorrectly added to the offset, which would cause the marker to be misaligned when the tab bar is scrolled.
| bestMarkerLeft = tabRect.right - groupRect.left + groupDiv.scrollLeft; | ||
| } else { | ||
| bestIndex = i; | ||
| bestMarkerLeft = tabRect.left - groupRect.left + groupDiv.scrollLeft; |
There was a problem hiding this comment.
The scrollLeft should not be added to bestMarkerLeft. Since the tab-insertion-mark is a child of .tab-bar (which is position: relative) and is not inside the scrollable .tab-group, its left position should be relative to the visible area of the tab bar. tabRect.left - groupRect.left already provides this viewport-relative offset. Adding scrollLeft will cause the marker to be incorrectly offset to the right when the tab bar is scrolled.
bestMarkerLeft = tabRect.right - groupRect.left;
} else {
bestIndex = i;
bestMarkerLeft = tabRect.left - groupRect.left;
There was a problem hiding this comment.
2 issues found across 7 files
Confidence score: 3/5
- There is a concrete UI behavior risk in
frontend/src/components/window/Panel.svelte: using+ groupDiv.scrollLeftfortab-insertion-markpositioning can misalign the insertion indicator when the tab strip is scrolled, which users will notice during tab drag/reorder. - The
pointerdownhandling infrontend/src/components/window/Panel.svelteshould be guarded so close-button clicks do not also activate a tab or start drag state, otherwise normal close interactions may feel broken or inconsistent. - Given one medium-severity placement bug with high confidence plus a second interaction bug, this carries some user-facing regression risk and is worth a quick fix before merge.
- Pay close attention to
frontend/src/components/window/Panel.svelte- tab insertion marker math and tabpointerdownclose-button guarding can cause incorrect drag/activation behavior.
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:73">
P2: Guard the tab pointerdown handler so clicking the close button doesn’t activate the tab or start drag state.</violation>
<violation number="2" location="frontend/src/components/window/Panel.svelte:163">
P2: The `+ groupDiv.scrollLeft` offset is incorrect here (and on the corresponding line below). The `tab-insertion-mark` is absolutely positioned relative to `.tab-bar`, not inside the scrollable `.tab-group`. Since `getBoundingClientRect()` already returns viewport-relative coordinates, `tabRect.right - groupRect.left` is the correct visual offset. Adding `scrollLeft` will cause the insertion marker to appear shifted to the right when the tab group is scrolled.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Partly closes #195.