feat(core): drag to move and corner handles to resize in the inspector - #382
feat(core): drag to move and corner handles to resize in the inspector#382bruno-begh wants to merge 2 commits into
Conversation
The inspector edits properties of the selected element but cannot move it, so nudging a headline a few pixels means going back to the agent. Dragging the body of a selected element now repositions it and the four corner handles resize it, with the opposite edge anchored. Both end up as ordinary set-style operations (translate, width, height), the same ones the panel already emits, so Save, undo and redo need no new machinery and the result is written back into the .tsx. Movement uses the standalone translate property rather than transform, so it composes with whatever transform the slide already declares. The canvas scale is resolved from the element's own canvas because the same slide also renders in the thumbnail rail and the overview at smaller scales. Verified with packages/core/tools/verify-drag-resize.mjs against the fixture added in apps/demo.
|
@bruno-begh is attempting to deploy a commit to the open-slide Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds direct dragging and corner resizing for selected elements in Inspect mode. Gestures apply normalized ChangesInspector interaction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant InspectorOverlay
participant DragResizeLayer
participant SelectedElement
participant SourceFile
InspectorOverlay->>DragResizeLayer: mount controls for selected element
DragResizeLayer->>SelectedElement: apply scaled pointer movement
DragResizeLayer->>SourceFile: persist translate, width, and height style edits
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
packages/core/src/app/components/inspector/drag-resize-layer.tsx (2)
230-246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
aria-labelonrole="presentation"is discarded.
role="presentation"strips semantics, so the label is never exposed. Either drop it or make the handle a real control (role="button",tabIndex={0}, keyboard nudge) if resizing should be reachable without a pointer.🤖 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 `@packages/core/src/app/components/inspector/drag-resize-layer.tsx` around lines 230 - 246, Update the resize handle element in the drag-resize layer so its accessibility semantics are consistent: remove the ineffective aria-label and presentation role, or convert it into an interactive button with keyboard focus and nudge behavior if keyboard resizing is required. Preserve the existing pointer-resize behavior and corner-specific labeling.
56-107: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle
pointercancelfor the body drag.Only
pointerdown/pointermove/pointerupare bound. If the gesture is cancelled (touch cancel, browser gesture takeover),dragstays non-null anddocument.body.style.cursor = 'grabbing'/userSelect = 'none'remain applied until an unrelatedpointeruparrives. Also considersetPointerCaptureon the anchor so moves outside the window keep tracking, matching whatResizeHandlealready does.♻️ Suggested wiring
window.addEventListener('pointerdown', onPointerDown, true); window.addEventListener('pointermove', onPointerMove, true); window.addEventListener('pointerup', onPointerUp, true); + window.addEventListener('pointercancel', onPointerCancel, true); return () => { window.removeEventListener('pointerdown', onPointerDown, true); window.removeEventListener('pointermove', onPointerMove, true); window.removeEventListener('pointerup', onPointerUp, true); + window.removeEventListener('pointercancel', onPointerCancel, true);with
const onPointerCancel = () => { const wasMoving = drag?.moved; const snapshot = drag?.snapshot; drag = null; if (!wasMoving) return; document.body.style.cursor = ''; document.body.style.userSelect = ''; const { anchor: el } = stateRef.current; if (el?.isConnected && snapshot) restoreInline(el, snapshot); };🤖 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 `@packages/core/src/app/components/inspector/drag-resize-layer.tsx` around lines 56 - 107, Update the body-drag handlers around onPointerDown and onPointerMove to add pointer-capture support on the anchor for the active pointer, and implement an onPointerCancel cleanup path that clears drag state, restores body cursor/userSelect, and restores the saved inline styles when movement began. Wire the cancel handler wherever the existing pointerup listener is registered, matching the cleanup behavior used by ResizeHandle.
🤖 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 @.changeset/inspector-drag-resize.md:
- Line 5: Update the changeset description to use present-tense, user-facing
wording instead of the imperative “Drag”; describe that users can reposition the
selected element by dragging it and resize it with the corner handles, with
changes saved back to source as style edits.
In `@packages/core/src/app/components/inspector/drag-resize-layer.tsx`:
- Around line 263-268: Update readTranslate and the set-style normalization
logic at
packages/core/src/app/components/inspector/drag-resize-layer.tsx:263-268 and
:281-288 to handle computed translate units safely: resolve non-pixel components
or abort the gesture, and preserve the optional third z component instead of
dropping it. Apply consistent unit and z-component handling in both sites.
- Around line 206-224: Handle pointer cancellation in both gesture flows in
packages/core/src/app/components/inspector/drag-resize-layer.tsx: at lines
206-224, add a pointercancel handler that removes the pointermove and pointerup
listeners and restores the inline snapshot; at lines 56-107, add a pointercancel
handler that clears drag, resets document.body cursor and userSelect, and
restores the snapshot. Wire each handler into its corresponding gesture cleanup
without changing normal pointerup behavior.
- Around line 174-203: Update the resize initialization around the `startW` and
`startH` calculations to derive dimensions from the element’s computed box in
the same box model used when writing `el.style.width` and `el.style.height`.
Account for padding and borders, and adjust for the element’s scale/transform as
needed so the first pointer move preserves the current rendered size without
jumping; keep the existing corner anchoring and resize calculations unchanged.
- Around line 294-304: Update suppressNextClick so the swallow handler calls
stopImmediatePropagation() instead of stopPropagation(), ensuring the
later-registered window click listener is not invoked while preserving
preventDefault and listener cleanup.
In `@packages/core/tools/verify-drag-resize.mjs`:
- Around line 117-145: Update the resize checks in the corner-handle scenario to
capture the heading’s initial bounding box and validate persisted width and
height deltas against the drag distance scaled by scale, rather than comparing
absolute height. Add a northwest-handle resize case that verifies both
dimensions change and the persisted translate values shift to preserve top/left
anchoring, using the existing resize and save/readback helpers.
- Around line 34-35: Wrap the Playwright test flow using browser and page in a
try/finally structure so cleanup always executes after failures. In the finally
block, nest the existing browser.close() and writeFileSync(SLIDE_FILE, ORIGINAL)
cleanup so restoring the fixture still occurs if browser.close() throws;
preserve the existing test behavior and cleanup symbols.
---
Nitpick comments:
In `@packages/core/src/app/components/inspector/drag-resize-layer.tsx`:
- Around line 230-246: Update the resize handle element in the drag-resize layer
so its accessibility semantics are consistent: remove the ineffective aria-label
and presentation role, or convert it into an interactive button with keyboard
focus and nudge behavior if keyboard resizing is required. Preserve the existing
pointer-resize behavior and corner-specific labeling.
- Around line 56-107: Update the body-drag handlers around onPointerDown and
onPointerMove to add pointer-capture support on the anchor for the active
pointer, and implement an onPointerCancel cleanup path that clears drag state,
restores body cursor/userSelect, and restores the saved inline styles when
movement began. Wire the cancel handler wherever the existing pointerup listener
is registered, matching the cleanup behavior used by ResizeHandle.
🪄 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: CHILL
Plan: Pro Plus
Run ID: acf60ddd-6ca6-436e-8965-20c30c790d69
📒 Files selected for processing (5)
.changeset/inspector-drag-resize.mdapps/demo/slides/verify-drag-resize/index.tsxpackages/core/src/app/components/inspector/drag-resize-layer.tsxpackages/core/src/app/components/inspector/inspect-overlay.tsxpackages/core/tools/verify-drag-resize.mjs
| '@open-slide/core': minor | ||
| --- | ||
|
|
||
| Drag the selected element to reposition it and use the corner handles to resize, saved back to source as style edits. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use present-tense changeset wording.
“Drag” is imperative. Use a user-facing present-tense description instead.
Proposed fix
-Drag the selected element to reposition it and use the corner handles to resize, saved back to source as style edits.
+Lets you reposition selected elements by dragging and resize them with corner handles.As per coding guidelines, changeset descriptions must be “one line, present-tense, describing what changed from a user's perspective.”
📝 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.
| Drag the selected element to reposition it and use the corner handles to resize, saved back to source as style edits. | |
| Lets you reposition selected elements by dragging and resize them with corner handles. |
🤖 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 @.changeset/inspector-drag-resize.md at line 5, Update the changeset
description to use present-tense, user-facing wording instead of the imperative
“Drag”; describe that users can reposition the selected element by dragging it
and resize it with the corner handles, with changes saved back to source as
style edits.
Source: Coding guidelines
| function readTranslate(el: HTMLElement): { x: number; y: number } { | ||
| const raw = getComputedStyle(el).translate; | ||
| if (!raw || raw === 'none') return { x: 0, y: 0 }; | ||
| const [x, y] = raw.split(' '); | ||
| return { x: Number.parseFloat(x ?? '0') || 0, y: Number.parseFloat(y ?? '0') || 0 }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate parsing assumes exactly two px components. Both helpers split the value and parseFloat the first two tokens, so percentages are reinterpreted as px and a z component is dropped.
packages/core/src/app/components/inspector/drag-resize-layer.tsx#L263-L268: resolve non-px units (or bail out of the gesture) when reading the computed base translate, and preserve a third component.packages/core/src/app/components/inspector/drag-resize-layer.tsx#L281-L288: mirror the same unit/z handling when normalising the value written to theset-styleop.
📍 Affects 1 file
packages/core/src/app/components/inspector/drag-resize-layer.tsx#L263-L268(this comment)packages/core/src/app/components/inspector/drag-resize-layer.tsx#L281-L288
🤖 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 `@packages/core/src/app/components/inspector/drag-resize-layer.tsx` around
lines 263 - 268, Update readTranslate and the set-style normalization logic at
packages/core/src/app/components/inspector/drag-resize-layer.tsx:263-268 and
:281-288 to handle computed translate units safely: resolve non-pixel components
or abort the gesture, and preserve the optional third z component instead of
dropping it. Apply consistent unit and z-component handling in both sites.
| function suppressNextClick(): void { | ||
| const swallow = (e: MouseEvent) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| window.removeEventListener('click', swallow, true); | ||
| }; | ||
| window.addEventListener('click', swallow, true); | ||
| // If no click follows (pointer left the window, gesture cancelled), don't | ||
| // leave the listener armed for the user's next real click. | ||
| setTimeout(() => window.removeEventListener('click', swallow, true), 300); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
stopPropagation() does not block the overlay's own click listener.
InspectOverlay registers onClick on window in the capture phase (inspect-overlay.tsx Line 73), and this swallow listener is added on the same target later, so it runs after it. stopPropagation only stops propagation to other nodes, not to co-registered listeners on window — the post-gesture click still reaches onClick and re-selects whatever is under the pointer. Use stopImmediatePropagation().
🐛 Proposed fix
const swallow = (e: MouseEvent) => {
e.preventDefault();
- e.stopPropagation();
+ e.stopImmediatePropagation();
+ e.stopPropagation();
window.removeEventListener('click', swallow, true);
};🤖 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 `@packages/core/src/app/components/inspector/drag-resize-layer.tsx` around
lines 294 - 304, Update suppressNextClick so the swallow handler calls
stopImmediatePropagation() instead of stopPropagation(), ensuring the
later-registered window click listener is not invoked while preserving
preventDefault and listener cleanup.
| const browser = await chromium.launch({ channel: 'chrome', headless: true }); | ||
| const page = await browser.newPage({ viewport: { width: 1600, height: 1000 } }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Always restore the fixture in a finally block.
Any failed Playwright action skips lines 149-150, leaving Chrome open and potentially leaving the saved .tsx fixture modified. Wrap the test flow in try/finally; nest cleanup so writeFileSync(SLIDE_FILE, ORIGINAL) still runs if browser.close() fails.
Also applies to: 149-150
🤖 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 `@packages/core/tools/verify-drag-resize.mjs` around lines 34 - 35, Wrap the
Playwright test flow using browser and page in a try/finally structure so
cleanup always executes after failures. In the finally block, nest the existing
browser.close() and writeFileSync(SLIDE_FILE, ORIGINAL) cleanup so restoring the
fixture still occurs if browser.close() throws; preserve the existing test
behavior and cleanup symbols.
| console.log('\n2. corner handle resizes and persists to source'); | ||
| { | ||
| const { scale } = await selectHeading(); | ||
| const handle = page.locator('[data-drag-resize-handle="se"]'); | ||
| const hb = await handle.boundingBox(); | ||
| check(!!hb, 'bottom right handle is visible'); | ||
| if (hb) { | ||
| const from = { x: hb.x + hb.width / 2, y: hb.y + hb.height / 2 }; | ||
| await page.mouse.move(from.x, from.y); | ||
| await page.mouse.down(); | ||
| for (let i = 1; i <= 10; i++) { | ||
| await page.mouse.move(from.x - 10 * i, from.y + 5 * i); | ||
| await page.waitForTimeout(16); | ||
| } | ||
| await page.mouse.up(); | ||
| await page.waitForTimeout(300); | ||
|
|
||
| const saved = await save(); | ||
| check(saved, 'Save button appeared after the resize'); | ||
|
|
||
| const after = readFileSync(SLIDE_FILE, 'utf8'); | ||
| check(/width:\s*'\d+px'/.test(after), 'width written into the .tsx'); | ||
| check(/height:\s*'\d+px'/.test(after), 'height written into the .tsx'); | ||
| const h = after.match(/height:\s*'(\d+)px'/); | ||
| if (h) { | ||
| const expected = 50 / scale; | ||
| const grew = Number.parseInt(h[1], 10); | ||
| check(grew > expected * 0.5, `height grew with the drag (${grew}px)`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the resize check validate actual size deltas and anchored resizing.
grew > expected * 0.5 compares an absolute final height, so a no-op can pass when the heading’s original height exceeds the threshold; width is never checked for a delta. Record the initial bounding box, assert persisted width/height changed by the expected scaled amounts, and add a northwest-handle case that asserts translate changes alongside size. This covers the required top/left anchoring contract.
🤖 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 `@packages/core/tools/verify-drag-resize.mjs` around lines 117 - 145, Update
the resize checks in the corner-handle scenario to capture the heading’s initial
bounding box and validate persisted width and height deltas against the drag
distance scaled by scale, rather than comparing absolute height. Add a
northwest-handle resize case that verifies both dimensions change and the
persisted translate values shift to preserve top/left anchoring, using the
existing resize and save/readback helpers.
A cancelled pointer, a touch the browser turns into a scroll or a device that goes away mid-gesture, never delivers pointerup. The resize handle left its window listeners attached, so the element kept resizing on every later pointer move, and both gestures left it wherever the gesture happened to die. Also seed the resize from the computed width and height instead of the rendered box. Under content-box the two differ by padding and border, and it is the computed value that means the same thing as the width being written back.
|
Addressed all three points, pushed in d6391db.
Resize seed. Also correct. I measured it rather than assume: with Changeset wording. Reworded, though not to the suggested phrasing. Every entry in I left the docstring coverage warning alone. CONTRIBUTING asks contributors to "default to writing no comments" and to document only the non-obvious why, which is what the file does. Re-verified after the change: |
1 participant
Closes #380.
Problem
Inspect mode edits everything about the selected element except the two most visual properties: where it sits and how big it is. Position and size are the only adjustments that have to leave the canvas, either back to the agent or into the
.tsxto guess a number, reload, and look again.Change
Dragging the body of the selected element moves it. Four corner handles resize it, with the opposite edge anchored.
Both gestures end as ordinary
set-styleoperations, the same ones the panel already emits, so Save, undo, redo and the write-back into the.tsxneed no new machinery:translatewidthandheight, plustranslatewhen a top or left corner is draggedThree things worth a reviewer's attention:
translateproperty rather thantransform, so it composes with whatevertransformthe slide already declares instead of overwriting it.closest('[data-osd-canvas]'). The thumbnail rail and the overview grid render the same slide at much smaller scales, and reading the wrong one multiplies every drag.bufferOpsruns, because that call snapshots the current inline value for undo. Without the restore, undo would return to the dragged position rather than the original one.A gesture ends with a click the overlay would otherwise read as "select whatever is under the pointer", so exactly one click is swallowed, and the listener disarms itself after 300ms if no click follows.
Testing
pnpm format:check,pnpm lint,pnpm typecheck: cleanpnpm test: 305 passinge2e/tests/inspector.spec.ts: 7 passingapps/demo: drag plus each of the four handles, then Save, undo and redo, checking the values written into the.tsxand that an existingtransformon the element survives.packages/core/tools/verify-drag-resize.mjsdrives the gestures against theverify-drag-resizefixture and asserts the source was rewritten with the expected geometry. Run it with the demo dev server up.No new dependencies.
The fixture slide and the verification script are there to make the behaviour checkable in a clone. Happy to drop either if you would rather keep the demo free of them.
Summary by CodeRabbit