Slides: live snap guides + align/distribute toolbar#209
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (38)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR implements snap guides, alignment, and distribution features for the slides editor. Snap guides appear as magenta lines during drag, indicating alignment targets. A new align/distribute toolbar provides buttons to align elements (left/center/right/top/middle/bottom) or distribute them equally (horizontal/vertical), with proper enable/disable logic based on selection size. ChangesSnap Guides and Alignment/Distribution for Slides Editor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Adds the task plan for making the existing slides snap engine visible (live alignment guides drawn during drag) and a multi-select align/distribute toolbar group. Snap math already lives in view/editor/snap.ts; this plan turns its winning candidate into a SnapGuide rendered on the overlay layer, then layers Google-Slides- parity align/distribute commands on top using the same store.batch commit path as drag. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Phase 1 of the alignment-guides feature. snapDelta now returns a
`guides` array alongside `{dx, dy}`, describing which alignment
line (slide centre or element edge) won the snap on each axis.
A future overlay layer needs this so it can draw the visible guide
lines that appear during a drag — without it, the renderer would
have to recompute the snap candidates itself.
The internal helper bestSnapAdjust now returns the winning
candidate index, so snapDelta can distinguish the slide-centre
candidate (always at index 0 by construction) from element-edge
candidates and pull the right `position` for each.
The existing caller in editor.ts destructures `{dx, dy}` and is
unchanged in this phase — it silently ignores the new `guides`
field. Phase 3 will wire it through.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Code review on the snapDelta refactor flagged two issues. The guide classification branched on `winnerIndex === 0` to decide slide-center vs. edge — correct only because the slide-center candidate happened to be pushed first. Reordering or inserting candidates would silently mis-classify guides with no test failure. The X/Y guide-building blocks were also mirror-image duplicates, easy to "fix X but forget Y". Attach the guide kind to the candidate itself, then `bestSnapAdjust` returns the winning candidate (with kind) instead of an index. Guide construction collapses to one shared `toGuide(axis, winner)` helper with no special-case for slide-center, since the slide-center candidate's `to` already equals slide.w/2 or slide.h/2. Also tighten the new snap.test.ts guide assertions from `toContainEqual` to full-array `toEqual`. This caught a phantom y-axis guide in the edge-snap test (the other element shared y=0 with the dragged bbox, accidentally emitting a zero-diff y snap); fixed by moving the test fixture to y=400 so only the intended axis snaps. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Phase 1 added a `guides: SnapGuide[]` field to the result of `snapDelta` so callers can know which alignment line(s) won the snap. This phase makes that information visible: `renderOverlay` now accepts optional `guides` plus the (now required) slide dimensions and draws one 1px magenta line per guide spanning the full slide width / height. The guides are rendered as DOM children of the existing overlay layer (absolute-positioned, `pointer-events: none`) so the canvas paint path is untouched and clearing the overlay on the next render naturally removes them. Only the axis-aligned `renderOverlay` path emits guides — rotated single-element selection skips them, since the drag handler always operates on the axis-aligned bbox and rotated handles are only shown while idle. `slideWidth`/`slideHeight` are required on `OverlayOptions` rather than defaulted; every call site (3 in `editor.ts`, several in tests) already has access to `SLIDE_WIDTH` / `SLIDE_HEIGHT` so plumbing them through is cheap and keeps the helper free of presentation-model imports. Phase 3 will wire `snapDelta(...).guides` through the drag handler and into this new field. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Phases 1 + 2 added snap guide info to `snapDelta()` and made the overlay renderer accept a `guides` array, but the drag handler in `editor.ts` still ignored both. Phase 3 connects them: capture `guides` from `snapDelta` on every mousemove, pass them through `paintLive` to `renderOverlay`, and refresh the overlay on mouseup so the magenta guide nodes disappear when the drop commits. `render()` only repaints the canvas, so an explicit `repaintOverlay()` is needed in `onUp` — without it the guide DOM nodes from the last `paintLive` would persist after the drag ends. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Phase 4 of the slides-guides-align plan. Pure helpers that compute new frames for the upcoming editor.align() / editor.distribute() API in Phase 5. alignFrames takes a reference rectangle (caller picks: combined bbox for multi-select, slide canvas for single-select) and one of six directions. distributeFrames spreads inner frames so consecutive gaps are equal while pinning the endpoints; it no-ops for fewer than three frames. Both return id->new-frame ONLY for frames that actually moved so the editor can keep store batches tight. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Phase 5 of slides-guides-align. Wires Phase 4's pure alignFrames /
distributeFrames into the SlidesEditor surface so the upcoming Phase 6
toolbar can call editor.align(direction) and editor.distribute(axis)
directly. Both methods follow the same store.batch + markDirty + render
pattern that drag/resize already use, ensuring align/distribute land as
a single atomic undo step rather than one undo per moved element.
Behavior:
- align: aligns to the combined bbox when ≥ 2 elements are selected,
and to the slide canvas (1920×1080) when exactly 1 is selected.
No-op for an empty selection.
- distribute: equalizes gaps between consecutive elements on the
given axis. No-op for fewer than 3 elements (undefined for 0/1/2).
- Both methods skip the store.batch entirely when the pure function
returns no moved frames, so we don't pollute undo history with
empty transactions.
The collectSelectedFrames helper defends against ids that may have been
removed remotely between selection and the toolbar action.
AlignDirection / DistributeAxis / AlignReference are re-exported from
the package barrel so the toolbar can type-check its dispatch table;
the alignFrames / distributeFrames helpers themselves stay internal.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Address two Important review follow-ups on the slides align/distribute
editor API:
- markDirty consistency: applyFrameUpdates was the only call site in
editor.ts using `this.markDirty()`. Every other site goes through
`this.renderer.markDirty()`. Switch it to match.
- Rotation contract: the JSDoc on `align` and `distribute` did not say
what happens to rotated elements. Add one sentence to each saying
the assigned position is written directly to `frame.x/y` and
`frame.rotation` is preserved (matches Google Slides). Lock that
contract in with one editor-level test that aligns two elements at
different rotations and asserts both `frame.rotation` values are
unchanged after `align('left')`.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Phase 6 of slides-guides-align: surfaces the SlidesEditor `align` and `distribute` actions landed in Phase 5 as a row of 8 toolbar buttons (6 align + 2 distribute) in the slides formatting toolbar. Align buttons are disabled when nothing is selected; distribute buttons require 3+ selected elements, mirroring Google Slides' ergonomics and the editor's no-op semantics so the disabled state communicates the requirement before the click. Selection size is tracked via a new state slot updated inside the existing selection/store refresh callback so remote peer edits (e.g., a collaborator deleting a selected element) keep the disabled state in sync without a second effect. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Document the snap-guide rendering (overlay layer, magenta 1px lines, explicit repaintOverlay clear) and the SlidesEditor.align/distribute API (multi-select bbox vs single-select slide reference, atomic store.batch, toolbar disabled-state rules). Capture two known v1 limitations: rotated multi-select align writes to unrotated frame fields with a rotation-aware reference, so rotated elements' visible left edges may not coincide; and idempotent distribute may emit sub-pixel float drift into the undo stack. Lessons file captures the seven cross-phase learnings, including the magic-index refactor, render() vs repaintOverlay asymmetry, and the implementer pushback on a wrong spec assumption that surfaced the rotated-align divergence. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
hackerwins
force-pushed
the
feat/slides-guides-align
branch
from
May 10, 2026 18:36
9e228a7 to
feafe10
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/slides/src/view/editor/overlay.ts (1)
40-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSnap guides are skipped for rotated single-element selections.
At Line 40 the rotated path returns at Line 42, so the guide rendering block at Line 85 is never reached. This drops live snap-guide feedback when dragging a single rotated element.
🔧 Suggested fix
export function renderOverlay( overlay: HTMLDivElement, selectedElements: readonly Element[], options: OverlayOptions, ): void { overlay.innerHTML = ''; if (selectedElements.length === 0) return; - if (selectedElements.length === 1 && selectedElements[0].frame.rotation !== 0) { - renderRotatedHandles(overlay, selectedElements[0].frame, options); - return; - } - - const bbox = combinedBoundingBox(selectedElements.map((e) => e.frame)); - if (!bbox) return; - - const { scale } = options; - const left = bbox.x * scale; - const top = bbox.y * scale; - const width = bbox.w * scale; - const height = bbox.h * scale; - - // Selection frame outline (no data-handle — purely decorative). - const frame = document.createElement('div'); - frame.className = 'wfb-slides-selection-frame'; - frame.style.position = 'absolute'; - frame.style.left = `${left}px`; - frame.style.top = `${top}px`; - frame.style.width = `${width}px`; - frame.style.height = `${height}px`; - frame.style.pointerEvents = 'none'; - frame.style.boxSizing = 'border-box'; - frame.style.border = '1px solid `#3a7`'; - overlay.appendChild(frame); - - const positions: Array<[string, number, number]> = [ - ['nw', left, top], - ['n', left + width / 2, top], - ['ne', left + width, top], - ['e', left + width, top + height / 2], - ['se', left + width, top + height], - ['s', left + width / 2, top + height], - ['sw', left, top + height], - ['w', left, top + height / 2], - ['rotate', left + width / 2, top - ROTATE_HANDLE_OFFSET], - ]; - for (const [kind, cx, cy] of positions) { - overlay.appendChild(makeHandle(kind, cx, cy)); + if (selectedElements.length === 1 && selectedElements[0].frame.rotation !== 0) { + renderRotatedHandles(overlay, selectedElements[0].frame, options); + } else { + const bbox = combinedBoundingBox(selectedElements.map((e) => e.frame)); + if (!bbox) return; + + const { scale } = options; + const left = bbox.x * scale; + const top = bbox.y * scale; + const width = bbox.w * scale; + const height = bbox.h * scale; + + const frame = document.createElement('div'); + frame.className = 'wfb-slides-selection-frame'; + frame.style.position = 'absolute'; + frame.style.left = `${left}px`; + frame.style.top = `${top}px`; + frame.style.width = `${width}px`; + frame.style.height = `${height}px`; + frame.style.pointerEvents = 'none'; + frame.style.boxSizing = 'border-box'; + frame.style.border = '1px solid `#3a7`'; + overlay.appendChild(frame); + + const positions: Array<[string, number, number]> = [ + ['nw', left, top], + ['n', left + width / 2, top], + ['ne', left + width, top], + ['e', left + width, top + height / 2], + ['se', left + width, top + height], + ['s', left + width / 2, top + height], + ['sw', left, top + height], + ['w', left, top + height / 2], + ['rotate', left + width / 2, top - ROTATE_HANDLE_OFFSET], + ]; + for (const [kind, cx, cy] of positions) { + overlay.appendChild(makeHandle(kind, cx, cy)); + } } // Snap guide lines ... if (options.guides && options.guides.length > 0) {🤖 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/slides/src/view/editor/overlay.ts` around lines 40 - 43, The early return after renderRotatedHandles skips the snap-guide rendering for single rotated selections; change the logic in the overlay handling so rotated single-element cases still run the snap-guide block — either remove the return after renderRotatedHandles or invoke the snap-guide renderer (e.g., renderSnapGuides or the existing guide rendering function) after calling renderRotatedHandles, ensuring you pass the same overlay and frame/options (selectedElements[0].frame, options) so live snap guides appear for rotated selections.
🤖 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 `@packages/frontend/src/app/slides/slides-formatting-toolbar.tsx`:
- Around line 136-139: The refresh() handler currently sets selectionSize from
editor.getSelection().length which can include ids removed remotely; change it
to compute the count from the current slide snapshot (the same collection used
by collectSelectedFrames/readSingleSelectedElement) so the toolbar reflects only
still-existing selected elements. In practice, inside refresh() use the existing
selection-reading helpers (e.g., call collectSelectedFrames(store, editor) or
filter editor.getSelection() against the current slide elements) and pass that
filtered length to setSelectionSize while still calling
setSelected(readSingleSelectedElement(store, editor)) so enablement for
align/distribute matches what the editor can act on.
In `@packages/slides/src/view/editor/editor.ts`:
- Around line 313-337: Both align() and distribute() can mutate frame geometry
while a text editor is mounted, causing the on-screen editor to diverge; add a
guard at the start of align() and distribute() that returns early when
this.editingElementId !== null (or call the existing finish/commitEditing method
if you prefer to end edit mode first). Locate the methods align and distribute,
check editingElementId, and skip performing
collectSelectedFrames/applyFrameUpdates when editingElementId is set to avoid
updating persisted frame positions while the inline text editor is active.
---
Outside diff comments:
In `@packages/slides/src/view/editor/overlay.ts`:
- Around line 40-43: The early return after renderRotatedHandles skips the
snap-guide rendering for single rotated selections; change the logic in the
overlay handling so rotated single-element cases still run the snap-guide block
— either remove the return after renderRotatedHandles or invoke the snap-guide
renderer (e.g., renderSnapGuides or the existing guide rendering function) after
calling renderRotatedHandles, ensuring you pass the same overlay and
frame/options (selectedElements[0].frame, options) so live snap guides appear
for rotated selections.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41f8500b-e3b0-4860-9460-ce834a8cf790
📒 Files selected for processing (14)
docs/design/slides/slides.mddocs/tasks/README.mddocs/tasks/active/20260511-slides-guides-align-lessons.mddocs/tasks/active/20260511-slides-guides-align-todo.mdpackages/frontend/src/app/slides/slides-formatting-toolbar.tsxpackages/slides/src/index.tspackages/slides/src/view/editor/align.test.tspackages/slides/src/view/editor/align.tspackages/slides/src/view/editor/editor.test.tspackages/slides/src/view/editor/editor.tspackages/slides/src/view/editor/overlay.test.tspackages/slides/src/view/editor/overlay.tspackages/slides/src/view/editor/snap.test.tspackages/slides/src/view/editor/snap.ts
Mark plan items complete and run pnpm tasks:archive — moves the todo and lessons files to docs/tasks/archive/2026/05/ and refreshes the active + archive indexes. The implementation work is shipped on this branch; the docs follow. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Contributor
Verification: verify:selfResult: ✅ PASS in 172.4s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Three findings from CodeRabbit: 1. align/distribute could mutate frame.x/y while a text-box editor was mounted, leaving the editor DOM at stale coords until the next mount cycle. Guard both methods with `editingElementId !== null`. New integration test in text-box-editor.test.ts pins the invariant via the existing mountTextBox mock. 2. The toolbar's selectionSize was the raw editor.getSelection().length, so a remote delete could leave align/distribute buttons enabled even though the editor would no-op. Filter against the current slide's elements in the refresh callback so the count matches what collectSelectedFrames in editor.ts will actually act on. 3. renderOverlay's rotated-handles branch returned before the guide render block, so a single rotated element being dragged got snap correction without visual feedback. Extract the axis-aligned handle path into renderAxisAlignedHandles and let the guide loop run after either branch. Slides tests: 399 (was 398). Frontend lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Adding 8 align + 2 distribute buttons to the slides formatting toolbar reflows every mobile slides harness scenario (toolbar height wraps differently) and shifts the dedicated slides-toolbar desktop capture. Same kind of refresh as PR #207 did for #206. 37 baselines updated: 33 slides mobile scenarios + 2 slides-toolbar desktop (light + dark.dark profile) + 4 root harness pages (`harness-visual.browser.{,desktop.dark,mobile,mobile.dark}.png`). Generated by `pnpm verify:browser:docker:update`. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
hackerwins
added a commit
that referenced
this pull request
May 11, 2026
Adds @wafflebase/slides as a third surface alongside Sheets and Docs, plus 53-shape library (Phase 1+2), adjustment handles for 9 pilot shapes (P3-A.1), live snap guides, align/distribute toolbar, themed authoring, and layout-change UI. Also ships Sheets cell comments (Phase B), docs peer-avatar caret jump, yorkie-js-sdk 0.7.8 upgrade, and CLI/REST API improvements (docs export imageFetcher, expired session refresh, REST docs split). Minor bump because slides is a new top-level package. Highlights: #184 #185 #186 #187 #188 #189 #190 #191 #192 #197 #198 #201 #202 #203 #204 #205 #206 #207 #209 #210
3 tasks
This was referenced May 24, 2026
10 tasks
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
snapDeltaemitsSnapGuide[]with axis/position/kind, and the overlay paints 1px magenta lines spanning the slide canvas. Guides clear on mouseup via an explicitrepaintOverlay()(sincerender()only repaints the canvas layer).SlidesEditor.align(direction)/distribute(axis)API plus 6 align + 2 distribute buttons in the slides toolbar. Multi-select uses the combined bbox; single-select uses the slide canvas. Distribute requires ≥3 elements. Each call commits in onestore.batchfor atomic undo.kind.Plan + lessons:
docs/tasks/active/20260511-slides-guides-align-todo.mdand…-lessons.md. Design doc updated atdocs/design/slides/slides.md("Snap guides + align / distribute" + Known limitations).Known limitations (documented, follow-ups)
combinedBoundingBoxover rotated AABBs), but values are written directly to the unrotatedframe.x/y, so visible left edges of rotated elements don't all coincide. Differs from Google Slides.!==no-op detection may emit sub-pixel moves on repeated calls. Acceptable for v1; consider EPSILON later.Test plan
pnpm --filter @wafflebase/slides test— 398/398 pass (was 371 before this branch; +27 new tests acrosssnap,overlay,align,editor).pnpm --filter @wafflebase/frontend lint— clean.pnpm verify:fast— green.🤖 Generated with Claude Code
Summary by CodeRabbit