Add Alt+Drag layer duplication to Layers Panel#3651
Conversation
|
This contains commits from other PRs. Please only include the relevant commit. |
|
sorry about that i will fix it now |
bac869e to
3774516
Compare
3774516 to
150c3c7
Compare
|
Please give this a try, it seems to be catastrophically broken (maybe something changed with the surrounding code as a result of the rebase?). |
150c3c7 to
21cb257
Compare
5bb6104 to
52d2b38
Compare
|
Hi, do you have any updates on this? Could you please resolve the merge conflict, confirm it works in the latest, and mark it as ready for review (and reply so I get an email about it)? I really do look forward to merging this feature, thank you! |
ok keavon i will resolve the merge conflicts and will test against the latest changes and will push an update shortly |
147a0d1 to
e48c2a1
Compare
There was a problem hiding this comment.
2 issues found across 2 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/panels/Layers.svelte">
<violation number="1" location="frontend/src/components/panels/Layers.svelte:109">
P2: The blur event listener uses an anonymous function and is never removed in `onDestroy`, causing a listener leak on each HMR cycle. Extract the callback into a named function (like the other handlers) and add the corresponding `removeEventListener` in `onDestroy`.</violation>
<violation number="2" location="frontend/src/components/panels/Layers.svelte:472">
P1: Race condition: `startDuplicates()` is async but not awaited. If Alt is released (or the drag is aborted) before `await tick()` resolves, `duplicatedLayerIds` is still `undefined`, so `stopDuplicates()` silently bails out — leaving orphaned duplicate layers in the document. Consider making the duplication fully synchronous, or guarding against this by tracking/awaiting the pending promise.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| addEventListener("keydown", draggingKeyDown); | ||
| addEventListener("keydown", handleLayerPanelKeyDown); | ||
| addEventListener("keyup", draggingKeyUp); | ||
| addEventListener("blur", () => internalDragState?.active && abortDrag()); |
There was a problem hiding this comment.
P2: The blur event listener uses an anonymous function and is never removed in onDestroy, causing a listener leak on each HMR cycle. Extract the callback into a named function (like the other handlers) and add the corresponding removeEventListener in onDestroy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/panels/Layers.svelte, line 109:
<comment>The blur event listener uses an anonymous function and is never removed in `onDestroy`, causing a listener leak on each HMR cycle. Extract the callback into a named function (like the other handlers) and add the corresponding `removeEventListener` in `onDestroy`.</comment>
<file context>
@@ -100,6 +105,8 @@
addEventListener("keydown", draggingKeyDown);
addEventListener("keydown", handleLayerPanelKeyDown);
+ addEventListener("keyup", draggingKeyUp);
+ addEventListener("blur", () => internalDragState?.active && abortDrag());
addEventListener("pointermove", clippingHover);
</file context>
e58c1de to
df8001f
Compare
9b97ab7 to
2e842cb
Compare
Looks like I'll need to ask for the same again since there are more conflicts. Also next time, please reply upon marking it as ready for review so I can be made aware of its change in status. THanks. |
e48c2a1 to
82c2b96
Compare
82c2b96 to
7fe9efb
Compare
@Keavon its ready for review would you review it |
f07c79b to
76938eb
Compare
|
Oh, this has still been marked as a draft so I never saw it or your comments about it. Marking as ready for review now for you. |
4b7a823 to
847b8e9
Compare
15fcaac to
d5f0140
Compare
|
Sorry it's taken so long to review this. It was a nice-to-have and not a critical-path item so it didn't make the cut for my day-to-day prioritization till now. I've given the branch a test and I'm noticing that it doesn't work when Alt is held before the drag, and in the case when it Alt is pressed during the drag, the behavior seems to also not behave correctly. If you'd like to fix that, feel free to reopen this PR, but I'll close it at least for now. Thanks for the attempt though! |
|
@Keavon Thanks for taking the time to review and test this. I appreciate the feedback. I'll take a closer look at the Alt-before-drag and Alt-during-drag behavior and reopen the PR once I've fixed those issues. |
Implements Alt+Drag duplication in Layers.svelte and exposes a WASM API for duplicating the current layer selection. Behavior matches the Select Tool, including modifier changes during drag and focus loss handling.
Fixes #2824