Add slides ruler + presentation-wide draggable guides#285
Merged
Conversation
Captures the v1 design for slides rulers (H/V, corner origin, inch/cm) and presentation-wide draggable guides, along with a six-phase implementation plan covering the docs ruler refactor, the slides ruler controller, the guide data model + Yorkie schema, interactions, snap integration, and read-only handling. The design intentionally defers text-box-local ruler mode and the View > Show ruler / Show guides toggles to a later release. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Split the monolithic Ruler class so its tick rendering and locale- aware unit math can be reused by the upcoming slides ruler without dragging docs-specific concepts (pages, margins, indent handles) along. `getGridConfig` now takes an optional `pxPerInch` so slides can pass its 144 dpi physical scale (1920 logical px / 13.333") instead of the hardcoded 96 dpi docs uses. `drawTicks` is a pure renderer that fills ticks + labels within a measured region, with an optional density override so the slides ruler can thin labels out when zoomed far below 1×. The docs Ruler class is otherwise unchanged: it still owns its margin-drag and indent-handle interactions and produces the same output as on main. All 792 unit tests in pnpm verify:fast pass. Prep for the slides ruler — see docs/design/slides/slides-ruler.md. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Add a SlidesRuler controller that paints horizontal + vertical ruler canvases on the slide stage. Reuses the docs tick / unit primitives through @wafflebase/docs with a slides-specific 144 dpi physical scale (1920 logical px / 13.333" PDF page). Tick density adapts to the editor zoom so the ruler stays legible from 0.25× to 3×. The editor takes optional hRulerCanvas / vRulerCanvas / rulerCorner refs; when supplied it instantiates the ruler and repaints it on every render() / setHostSize() call. SlidesView wires the new DOM elements at the corner + top + left edges of the slide stage and offsets the slide canvas by RULER_SIZE to make room. Display-only — drag-out guides and snap integration land in follow-up phases. See docs/design/slides/slides-ruler.md. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Introduce a Guide type and `SlidesDocument.guides: Guide[]`, store API (addGuide / moveGuide / removeGuide), Mem and Yorkie implementations, and a passive overlay render so existing guides paint as 1-px magenta lines spanning the slide canvas. Guides live at the presentation level (not per-slide), matching the Google Slides / PowerPoint convention. The Yorkie adapter lazy-inits `root.guides` on attach so pre-v0.4.2 documents load without a migration script. migrateDocument and the backend readSlidesRoot also default the field to `[]` so the read path stays shape-stable across pre- / post-ruler decks. User interactions (drag-out from rulers, guide drag, deletion) and snap integration land in subsequent phases. See docs/design/slides/slides-ruler.md. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Add ruler / guide pointer interactions: - Mousedown on either ruler canvas seeds a pending guide that follows the cursor; mouseup inside the slide commits via store.addGuide, mouseup outside cancels. - Mousedown within 4 px of an existing guide starts a move drag; mouseup over the slide commits store.moveGuide (no-op when the position is unchanged), mouseup over either ruler removes the guide. - Hover within 4 px of a guide swaps the canvas cursor to col-resize / row-resize so the affordance is discoverable without instructions. - Right-click on a guide opens a 3-item context menu (delete, delete all on this axis, delete all guides). All in-flight drags paint a half-opacity preview through the overlay layer so the user can see the gesture before commit; the committed guide is suppressed during its own move so the user does not see a double line. Drag math clamps to the slide extent and the draggingGuide presence field is reserved for the eventual peer preview wire-up. See docs/design/slides/slides-ruler.md. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Extend SnapGuide with a third 'guide' kind and feed each presentation- wide Guide into snapDelta as three candidate offsets per axis (left / right / centre for vertical guides; top / bottom / centre for horizontal). The winner-selection now honours a fixed priority — slide-center > guide > edge — so an explicit guide beats a nearby element edge inside the same 8-px threshold. When a drag lands on a permanent guide the overlay thickens that guide to 2 px in a deeper magenta instead of overlaying a separate dashed snap-indicator on top, keeping the visual uncluttered. Snap entries of kind 'guide' suppress the regular makeGuide overlay for the same reason. Keyboard nudge keeps its existing snap-less path; the new parameter on snapDelta is optional so callers that don't pass guides get the prior behaviour byte-for-byte. See docs/design/slides/slides-ruler.md. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Verify the v1 ruler honours the read-only mount contract: - Ruler-canvas pointerdown is bound inside `attachInteractions()` (gated by `options.readOnly`), so a viewer-role mount never starts a drag-out gesture. - Guide hit-test on the slide canvas also lives inside `attachInteractions()`'s pointerdown router, so an existing guide cannot be moved or deleted from a viewer mount. Add two regression tests that seed a guide, mount the editor with `readOnly: true`, dispatch pointerdown / pointerup, and assert the `guides[]` array is unchanged. Presenter and thumbnail paths already paint through `SlideRenderer` without involving `renderOverlay`, so guides are automatically absent from presentation mode and from per-slide thumbnails — no new code needed there. Slides PDF export is not yet implemented; when it lands it follows the same canvas-only path. Reconcile the design doc with what shipped: the ruler markers and per-drag position labels are moved to v1.1, snap differentiation is "thicken + deepen" rather than a separate dashed indicator, and the in-flight preview is local-only in v1 (the `draggingGuide` presence slot is reserved for the v1.1 broadcast wiring). Closes the work tracked in docs/tasks/active/20260523-slides-ruler-todo.md. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
A 20-px gray ruler bar around the slide treats the canvas like a measurable document. Slides is closer to a stage, so the chrome should defer to the slide elevation rather than frame it. Tone-down the slides ruler: - RULER_SIZE drops 20 → 14, reclaiming ~30 % of the canvas gutter on the top + left edges. - Background fill is removed (clearRect instead) so the ruler inherits the column color. The corner is transparent too — no more gray block at the slide's top-left. - Tick color shifts from #999 to #b0b0b0; labels shrink from 9 px to 8 px Arial and sit at y = 0 (was y = 1) so they don't crowd the major tick. drawTicks gains optional `tickHeights`, `labelInset`, and `verticalLabelInset` params (defaults preserve docs behaviour bit-for- bit). Slides passes a 6 / 4 / 2 tick height and inset 0 / 4 so the slim ruler renders cleanly without label / tick overlap. The 20 → 14 cascade propagates through `SLIDES_RULER_SIZE` to slides-view's canvas offset and resize observer math without any further edits. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Two layout fixes for the slides editor shell: - The slide used to hug the top edge of the canvas column (`alignSelf: flex-start`), leaving an awkward gap below on tall viewports. Wrap the canvas in a new `canvasArea` flex child that fills the remaining vertical space and centers the canvas both horizontally and vertically — matches the Google Slides feel. - Add a horizontal resizer between the canvas area and the speaker- notes panel. Drag up to expand notes, drag down to shrink, mirrored off the existing thumbnail-panel divider's visual + interaction pattern (1-px hairline that thickens on hover, mousedown latches the gesture against document-level mousemove/mouseup). The notes height is a controlled state with a 60-px floor, a 60 % column-height ceiling, and `localStorage` persistence under `wfb-slides-notes-height` — the same shape the leftWidth handle uses. Pulling the ResizeObserver math into a `refitCanvas` function lets the notes drag rerun the fit without faking a ResizeObserver tick (the column's outer size hasn't changed during a notes drag, so the observer would never fire). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The notes textarea was rendered as a bordered box with its own intrinsic minHeight and a native resize handle — a holdover from before the slides editor shell had its own drag affordance for the notes height. Strip the box: no border, no border-radius, transparent background, disable native resize. The textarea now stretches to `height: 100%` so the user's drag of the notes resizer above directly grows the editable surface. Padding + foreground color stay so the typing area still has breathing room and follows the active theme. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Move the H / V ruler canvases out of canvasWrap (which holds the slide + overlay) and into canvasArea (the centred flex container). The ruler now hugs the canvas-area's top + left edges with `right: 0` / `bottom: 0`, so it stays put as the slide drifts inside the frame — either because the column resizes or because the user drags the speaker-notes panel taller. Mirrors how Photoshop / Figma anchor rulers to the workspace edge rather than to the artwork. The slide centring is unchanged; the canvas area picks up `padding-top: RULER_SIZE; padding-left: RULER_SIZE` so the slide centres inside the *non-ruler* portion of the frame. `SlidesRuler` reads each ruler canvas's own `clientWidth` / `clientHeight` to size its backing store, and projects the slide's tick "zero" via `(frameWidth - hostWidth) / 2` (the same maths the flex centring applies). CSS dimensions on the canvases are now the host's responsibility — the absolute `right: 0` / `bottom: 0` pins would otherwise be overwritten on every render. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
A bare `position: absolute; left: 14px; right: 0;` doesn't grow a canvas element to fill the gap — canvases fall back to their bitmap intrinsic dimensions, so the ruler collapsed into a tiny sliver at the top-left of canvasArea. Set the CSS `width` (h-ruler) and `height` (v-ruler) explicitly: - Seed an initial value at mount so the first paint sees a sensible size before the ResizeObserver fires. - Recompute on every `refitCanvas` tick from `canvasArea.getBoundingClientRect()` so a notes drag (which changes canvasArea height but not the right column's overall size) still resizes the vertical ruler. The `right: 0` / `bottom: 0` declarations are now redundant and removed. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The previous commit referenced hostW / hostH while seeding the ruler canvas dimensions, but those bindings were declared lower down in the same effect — TDZ blew up the React mount with "Cannot access 'hostW' before initialization". Hoist the `let hostW = initial.width; let hostH = initial.height;` to before the ruler DOM construction so the seed reads valid numbers. canvasWrap dimensions move in the same block; refit then overwrites them on the first ResizeObserver tick as before. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
📝 WalkthroughWalkthroughAdds display rulers and presentation-wide alignment guides with shared tick/unit renderer, persistent Yorkie-backed storage, editor wiring for drag/create/move/delete, snap engine integration, overlay rendering, and tests/docs/tasks. ChangesSlides Ruler Feature with Guides and Shared Rendering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
Contributor
Verification: verify:selfResult: ✅ PASS in 207.7s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/slides/src/view/editor/editor.ts (2)
2383-2389:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep permanent guides visible in every live overlay repaint.
Both of these preview paths rebuild
overlay.innerHTMLwithout passingpermanentGuides/pendingGuide, so presentation guides disappear while connector-endpoint drags or adjustment drags are in flight.Proposed fix
renderOverlay(this.options.overlay, selected, { scale: this.scale(), slideWidth: SLIDE_WIDTH, slideHeight: SLIDE_HEIGHT, allElements: startSlide.elements, connectorAffordance: this.connectorAffordance(), + permanentGuides: this.options.store.read().guides, + pendingGuide: this.pendingGuide, });renderOverlay(this.options.overlay, selected, { scale: this.scale(), slideWidth: SLIDE_WIDTH, slideHeight: SLIDE_HEIGHT, allElements: synthetic.elements, + permanentGuides: this.options.store.read().guides, + pendingGuide: this.pendingGuide, });Also applies to: 2539-2544
🤖 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/editor.ts` around lines 2383 - 2389, The live overlay repaint paths call renderOverlay(this.options.overlay, selected, {...}) but omit passing the current permanentGuides and pendingGuide, causing presentation guides to vanish during connector/adjustment drags; update those renderOverlay invocations (the ones using this.options.overlay and selected with scale/slideWidth/slideHeight/allElements/connectorAffordance) to include the active guide state by passing permanentGuides: this.permanentGuides and pendingGuide: this.pendingGuide (or the equivalent properties on the component) into the options object so permanent guides remain visible during live preview repaints.
1089-1119:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCancel in-flight guide drags when the editor detaches.
These guide drags are started fire-and-forget, and the surrounding comments say the helpers install their own
documentlisteners.detach()only unregisters handlers tracked viathis.on(...), so navigating away mid-drag can leave guide listeners running against a disposed editor/store untilpointeruphappens.Please verify whether the ruler interaction helpers expose teardown or accept an
AbortSignal, then thread that cancellation intodetach().#!/bin/bash set -euo pipefail editor_file="packages/slides/src/view/editor/editor.ts" interactions_file="packages/slides/src/view/editor/ruler/interactions.ts" echo "== editor attach/detach ==" sed -n '1089,1258p' "$editor_file" echo echo "== guide interaction helpers ==" sed -n '1,260p' "$interactions_file" echo echo "== listener lifecycle markers ==" rg -n -C2 'startGuideMove|startRulerDragOut|AbortController|AbortSignal|addEventListener|removeEventListener|pointermove|pointerup' \ "$editor_file" "$interactions_file"Expected result: if the helper functions add
documentlisteners and neither return a disposer nor consume an abort signal, the teardown hole is confirmed.Also applies to: 1177-1193
🤖 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/editor.ts` around lines 1089 - 1119, The detach() path currently leaves in-flight guide/ruler drags running because helpers like startGuideMove and startRulerDragOut install their own document listeners but aren't cancelled; update the ruler interaction lifecycle so detach() cancels them: inspect startGuideMove/startRulerDragOut in ruler/interactions.ts and either (A) make them accept an AbortSignal parameter or (B) make them return a disposer function/object, then store that AbortController or disposer on the editor instance (e.g. this.activeGuideAbort or this.activeGuideDisposers) when starting a drag and call abort() or invoke the disposer inside detach() (alongside this.ruler?.dispose()) to remove any document listeners and prevent callbacks after teardown. Ensure the symbol names startGuideMove, startRulerDragOut, detach, and this.ruler?.dispose are used to locate code to change.
🧹 Nitpick comments (3)
packages/slides/test/view/editor/snap.test.ts (1)
89-94: ⚡ Quick winConsider asserting array length before accessing element.
For clearer test failures, assert that
result.guideshas the expected length before accessingresult.guides[0]. If the array is unexpectedly empty, the current pattern will produce a less clear error message.🧪 Proposed pattern for clearer test assertions
For example, at lines 89-94:
expect(result.dx).toBe(40); + expect(result.guides).toHaveLength(1); expect(result.guides[0]).toMatchObject({ axis: 'x', position: 900, kind: 'guide', guideId: 'g1', });Apply the same pattern at lines 109, 124-127, and 136-141.
Also applies to: 109-109, 124-127, 136-141
🤖 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/test/view/editor/snap.test.ts` around lines 89 - 94, Add explicit length assertions before indexing into result.guides to make failures clearer: for each assertion that accesses result.guides[0] (the block that currently checks axis/position/kind/guideId) and the similar assertions later in the test suite, insert expect(result.guides).toHaveLength(<expectedCount>) immediately prior to accessing element 0 (and use toHaveLength for other indexed assertions as appropriate); update the test cases that reference result.guides (the first expect block checking axis/position/kind/guideId and the subsequent blocks at the other guide assertions) so each indexes only after the length assertion.packages/backend/src/yorkie/slides-tree.spec.ts (1)
76-76: ⚡ Quick winAdd an empty-root default assertion for
guides.Since
guidesis now part of the canonical slides shape, please also assertexpect(result.guides).toEqual([])in the empty-root test to cover the backward-compatibility path.As per coding guidelines: "
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests for critical business logic and edge cases".🤖 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/backend/src/yorkie/slides-tree.spec.ts` at line 76, Update the "empty-root" test in slides-tree.spec.ts to assert the new canonical field by adding expect(result.guides).toEqual([]) alongside the existing assertions; locate the test that builds/returns result (look for the test name or variable result in the empty-root test) and add the guides assertion to cover the backward-compatibility path so the spec verifies that guides defaults to an empty array.packages/slides/src/view/editor/ruler/interactions.ts (1)
101-128: ⚡ Quick winTear down drag state fully on cancel/dispose.
Current disposers only remove listeners. If a drag is interrupted before
pointerup(e.g., unmount/cancel), pending guide state and body cursor can remain stale. Add unified cleanup andpointercancelhandling in both drag flows.💡 Proposed direction
const onMove = (e: PointerEvent) => { @@ }; + const cleanup = () => { + document.removeEventListener('pointermove', onMove); + document.removeEventListener('pointerup', onUp); + document.removeEventListener('pointercancel', onCancel); + host.setBodyCursor(null); + host.setPendingGuide(null); + }; + const onCancel = () => cleanup(); const onUp = (e: PointerEvent) => { - document.removeEventListener('pointermove', onMove); - document.removeEventListener('pointerup', onUp); - host.setBodyCursor(null); const { x, y } = host.clientToLogical(e.clientX, e.clientY); if (host.isInsideSlide(x, y)) { const position = clamp(axis === 'x' ? x : y, axis); host.commitAddGuide(axis, position); } - host.setPendingGuide(null); + cleanup(); }; document.addEventListener('pointermove', onMove); document.addEventListener('pointerup', onUp); + document.addEventListener('pointercancel', onCancel); @@ - return () => { - document.removeEventListener('pointermove', onMove); - document.removeEventListener('pointerup', onUp); - }; + return cleanup;Apply the same cleanup pattern to
startGuideMove.Also applies to: 144-179
🤖 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/ruler/interactions.ts` around lines 101 - 128, The drag disposers currently only remove pointer listeners which leaves stale state (pending guide and body cursor) if a drag is cancelled/unmounted; update the cleanup to be a unified function that removes 'pointermove', 'pointerup', and 'pointercancel' listeners and also calls host.setPendingGuide(null) and host.setBodyCursor(null); use that cleanup in the returned disposer and in the onUp/onCancel handlers (replace the inline document.removeEventListener calls in onUp with a call to the shared cleanup), and apply the same pattern inside startGuideMove so both guide creation and guide moving consistently clear state and listeners.
🤖 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 `@docs/design/slides/slides-ruler.md`:
- Around line 70-80: The markdown fenced code blocks in the slides (the package
tree block showing "packages/docs/src/view/ruler/ ..." and the editor-shell
snippet showing "<slide-stage> ..." ) are missing language identifiers and
trigger MD040; update both backtick fences to include a language tag (e.g.,
```text) so markdownlint passes. Locate the two fenced blocks that contain the
package tree and the editor-shell/ruler canvas snippet (the blocks around the
Ruler file list and the <slide-stage> layout) and add the same explicit language
identifier to each opening fence.
In `@packages/frontend/src/app/slides/slides-view.tsx`:
- Around line 480-487: refitCanvas() must clamp the restored notesHeight against
MAX_NOTES_H_RATIO so a value loaded from localStorage can't exceed the current
column cap; before computing reservedBelow (and before using notesHeight to
compute availH), compute a clampedNotesHeight = Math.min(notesHeight,
rightRect.height * MAX_NOTES_H_RATIO) (or similar) and use clampedNotesHeight
for reservedBelow and any layout math; update any logic that reads notesHeight
from storage (and the refitCanvas body) to apply this clamp so the cap is
enforced on resize/reload without requiring a drag.
In `@packages/frontend/src/app/slides/yorkie-slides-store.ts`:
- Around line 1476-1505: The addGuide, moveGuide, and removeGuide mutations
modify this.doc but the undo/redo restore path (replaceRoot / snapshot
restoration) doesn’t include snapshot.guides, so guide changes aren’t reverted;
update the restore logic used by replaceRoot (or the snapshot builder used for
undo/redo) to include snapshot.guides (and preserve id, axis, position) so that
undo/redo fully restores guide arrays, and ensure the guide shape/types match
what addGuide/moveGuide/removeGuide write (referencing functions addGuide,
moveGuide, removeGuide and the snapshot/replaceRoot restore path) so guide state
is captured and restored during history operations.
In `@packages/slides/src/model/migrate.ts`:
- Around line 32-37: migrateGuide currently returns loose values (any) allowing
id to be undefined and position to be NaN/Infinity; update migrateGuide to
return a properly typed Guide: ensure id is a non-empty string (if g?.id is not
a string or empty, synthesize a stable fallback id e.g. via crypto.randomUUID()
or project createId helper), normalize axis to 'x'|'y' as you already do, and
coerce/validate position with Number.isFinite(g?.position) ? Number(g.position)
: 0; change the function signature to return Guide (not any) so callers like
store.moveGuide/removeGuide, overlay, and snap consume safe values and avoid
throwing or propagating NaN.
In `@packages/slides/src/store/memory.ts`:
- Around line 878-891: Reject non-finite positions in addGuide and moveGuide by
validating the position with Number.isFinite(position) before mutating state; in
addGuide (where Guide is created) and in moveGuide (before assigning
guide.position) call requireBatch(), then if Number.isFinite(position) is false
throw an Error like `Invalid guide position: ${position}`, otherwise proceed
(use existing symbols addGuide, moveGuide, generateId, this.doc.guides,
GuideAxis, guide.position).
In `@packages/slides/src/view/editor/notes-panel.ts`:
- Around line 55-56: The code removes the textarea outline (ta.style.outline =
'none'), which breaks keyboard focus visibility; instead, keep a focus indicator
by replacing the blanket outline removal with a focus-visible style: add a CSS
class (eg. notes-textarea) or inject a stylesheet for the textarea that sets
outline: none only for :focus (not :focus-visible) and defines a subtle
accessible focus-visible rule (e.g., thin high-contrast ring or box-shadow) that
preserves the borderless look when unfocused but shows a clear indicator when
the element has keyboard focus; update the code that sets ta.style.outline to
apply the class or ensure the injected focus-visible CSS is present so ta gains
the new focus-visible styling.
In `@packages/slides/src/view/editor/overlay.ts`:
- Around line 118-124: The highlighted snapped guide is widened to 2px but not
repositioned, shifting the visible line by 0.5px; update the overlay logic that
handles snappedGuideIds (where g.axis === 'x' sets el.style.width and else sets
el.style.height) to also offset the element’s position so the 2px stroke is
centered on the snapped coordinate: when g.axis === 'x' set el.style.left to the
snapped coordinate minus 1px (using g.coord or the existing coordinate source),
and when axis !== 'x' set el.style.top to the snapped coordinate minus 1px,
keeping the rest of the visual styling intact.
In `@packages/slides/src/view/editor/ruler/interactions.ts`:
- Around line 69-80: hitTestGuide currently returns the first guide within
GUIDE_HIT_PX instead of the closest; modify hitTestGuide to scan all guides and
track the minimum distance and corresponding guide (using g.axis and g.position
with point.x/point.y) and after the loop return the guide with the smallest
distance if that distance <= GUIDE_HIT_PX, otherwise return null. Ensure types
remain Guide | null and behavior unchanged when no guide is within threshold.
---
Outside diff comments:
In `@packages/slides/src/view/editor/editor.ts`:
- Around line 2383-2389: The live overlay repaint paths call
renderOverlay(this.options.overlay, selected, {...}) but omit passing the
current permanentGuides and pendingGuide, causing presentation guides to vanish
during connector/adjustment drags; update those renderOverlay invocations (the
ones using this.options.overlay and selected with
scale/slideWidth/slideHeight/allElements/connectorAffordance) to include the
active guide state by passing permanentGuides: this.permanentGuides and
pendingGuide: this.pendingGuide (or the equivalent properties on the component)
into the options object so permanent guides remain visible during live preview
repaints.
- Around line 1089-1119: The detach() path currently leaves in-flight
guide/ruler drags running because helpers like startGuideMove and
startRulerDragOut install their own document listeners but aren't cancelled;
update the ruler interaction lifecycle so detach() cancels them: inspect
startGuideMove/startRulerDragOut in ruler/interactions.ts and either (A) make
them accept an AbortSignal parameter or (B) make them return a disposer
function/object, then store that AbortController or disposer on the editor
instance (e.g. this.activeGuideAbort or this.activeGuideDisposers) when starting
a drag and call abort() or invoke the disposer inside detach() (alongside
this.ruler?.dispose()) to remove any document listeners and prevent callbacks
after teardown. Ensure the symbol names startGuideMove, startRulerDragOut,
detach, and this.ruler?.dispose are used to locate code to change.
---
Nitpick comments:
In `@packages/backend/src/yorkie/slides-tree.spec.ts`:
- Line 76: Update the "empty-root" test in slides-tree.spec.ts to assert the new
canonical field by adding expect(result.guides).toEqual([]) alongside the
existing assertions; locate the test that builds/returns result (look for the
test name or variable result in the empty-root test) and add the guides
assertion to cover the backward-compatibility path so the spec verifies that
guides defaults to an empty array.
In `@packages/slides/src/view/editor/ruler/interactions.ts`:
- Around line 101-128: The drag disposers currently only remove pointer
listeners which leaves stale state (pending guide and body cursor) if a drag is
cancelled/unmounted; update the cleanup to be a unified function that removes
'pointermove', 'pointerup', and 'pointercancel' listeners and also calls
host.setPendingGuide(null) and host.setBodyCursor(null); use that cleanup in the
returned disposer and in the onUp/onCancel handlers (replace the inline
document.removeEventListener calls in onUp with a call to the shared cleanup),
and apply the same pattern inside startGuideMove so both guide creation and
guide moving consistently clear state and listeners.
In `@packages/slides/test/view/editor/snap.test.ts`:
- Around line 89-94: Add explicit length assertions before indexing into
result.guides to make failures clearer: for each assertion that accesses
result.guides[0] (the block that currently checks axis/position/kind/guideId)
and the similar assertions later in the test suite, insert
expect(result.guides).toHaveLength(<expectedCount>) immediately prior to
accessing element 0 (and use toHaveLength for other indexed assertions as
appropriate); update the test cases that reference result.guides (the first
expect block checking axis/position/kind/guideId and the subsequent blocks at
the other guide assertions) so each indexes only after the length assertion.
🪄 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: 403c281b-9f4c-463b-b59e-a4fe5c80053b
📒 Files selected for processing (40)
docs/design/README.mddocs/design/slides/slides-ruler.mddocs/tasks/README.mddocs/tasks/active/20260523-slides-ruler-todo.mdpackages/backend/src/yorkie/slides-tree.spec.tspackages/backend/src/yorkie/slides-tree.tspackages/docs/src/index.tspackages/docs/src/view/editor.tspackages/docs/src/view/ruler/index.tspackages/docs/src/view/ruler/tick-renderer.tspackages/docs/src/view/ruler/unit.tspackages/docs/test/view/ruler.test.tspackages/frontend/src/app/slides/slides-view.tsxpackages/frontend/src/app/slides/yorkie-slides-store.tspackages/frontend/src/types/slides-document.tspackages/frontend/src/types/users.tspackages/slides/src/import/pptx/index.tspackages/slides/src/index.tspackages/slides/src/model/migrate.tspackages/slides/src/model/presentation.tspackages/slides/src/node.tspackages/slides/src/store/memory.tspackages/slides/src/store/store.tspackages/slides/src/view/canvas/layout-preview.tspackages/slides/src/view/editor/editor.tspackages/slides/src/view/editor/notes-panel.tspackages/slides/src/view/editor/overlay.tspackages/slides/src/view/editor/ruler/index.tspackages/slides/src/view/editor/ruler/interactions.tspackages/slides/src/view/editor/ruler/ruler.tspackages/slides/src/view/editor/snap.tspackages/slides/test/store/memory.test.tspackages/slides/test/view/canvas/element-renderer.test.tspackages/slides/test/view/canvas/group-render.test.tspackages/slides/test/view/canvas/slide-renderer.test.tspackages/slides/test/view/canvas/thumbnail.test.tspackages/slides/test/view/editor/editor.test.tspackages/slides/test/view/editor/ruler/interactions.test.tspackages/slides/test/view/editor/ruler/ruler.test.tspackages/slides/test/view/editor/snap.test.ts
At zoom-to-fit the slide canvas was butting right up against the H/V rulers — the 12-px drop shadow blurred into the tick marks and the 1-px slide outline overlapped the ruler edge, making the chrome look smudgy. Subtract a 12-px gap from `slideAvailW` / `slideAvailH` inside `refitCanvas` so the slide is sized to leave a constant margin from every ruler / canvas-area edge. The flex centring on `canvasArea` distributes the freed space evenly, and the existing `(frame - host) / 2` slide-offset math inside `SlidesRuler` keeps tick "0" aligned with the slide's new left / top edge — no ruler-side changes needed. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/frontend/src/app/slides/yorkie-slides-store.ts (1)
1476-1505:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuide mutations are not fully undo/redo-safe yet.
These new guide writes are batched/snapshotted, but
replaceRoot(Line 472+) still doesn’t restoresnapshot.guides, so guide add/move/remove won’t reliably revert.💡 Proposed fix
private replaceRoot(snapshot: SlidesDocument): void { this.doc.update((r) => { r.meta = clone(snapshot.meta); - const rootAny = r as { themes?: Theme[]; masters?: Master[] }; + const rootAny = r as { + themes?: Theme[]; + masters?: Master[]; + guides?: Array<{ id: string; axis: 'x' | 'y'; position: number }>; + }; rootAny.themes = clone(snapshot.themes); rootAny.masters = clone(snapshot.masters); + rootAny.guides = clone(snapshot.guides); const nextSlides: YorkieSlide[] = snapshot.slides.map((s) => ({🤖 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/frontend/src/app/slides/yorkie-slides-store.ts` around lines 1476 - 1505, The guide mutations (addGuide, moveGuide, removeGuide) are batched but undo/redo fails because replaceRoot doesn't restore snapshot.guides; update the logic in replaceRoot to include snapshot.guides in the restored state (ensure you copy/assign snapshot.guides onto the new root object rather than ignoring it), preserve types for guides (Array<{id: string; axis: 'x'|'y'; position: number}>) and use a defensive clone when applying snapshot.guides so subsequent mutations don't share references with the snapshot.packages/slides/src/store/memory.ts (1)
878-891:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard guide positions against non-finite numbers.
addGuideandmoveGuideacceptNaN/Infinity(Line 878 and Line 886), which can poison downstream snap/overlay geometry.💡 Proposed fix
addGuide(axis: GuideAxis, position: number): string { this.requireBatch(); + if (!Number.isFinite(position)) { + throw new Error(`Invalid guide position: ${position}`); + } const id = generateId(); const guide: Guide = { id, axis, position }; this.doc.guides.push(guide); return id; } moveGuide(id: string, position: number): void { this.requireBatch(); + if (!Number.isFinite(position)) { + throw new Error(`Invalid guide position: ${position}`); + } const guide = this.doc.guides.find((g) => g.id === id); if (!guide) throw new Error(`Guide not found: ${id}`); guide.position = position; }🤖 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/store/memory.ts` around lines 878 - 891, Validate that the position is a finite number in both addGuide and moveGuide: before creating/adding the Guide in addGuide (function addGuide) and before assigning guide.position in moveGuide (function moveGuide), check Number.isFinite(position) and throw a clear error (e.g., "Invalid guide position") if it isn't; keep the existing requireBatch() calls and error behavior for missing guide in moveGuide.
🧹 Nitpick comments (2)
packages/backend/src/yorkie/slides-tree.spec.ts (1)
76-76: ⚡ Quick winAdd an explicit default assertion for
guidesin the empty-root read test.Now that
guidesis part of the canonical top-level shape, addexpect(result.guides).toEqual([])in thereturns sensible defaults when reading an empty rootcase to lock migration/default behavior.As per coding guidelines, “Write tests for critical business logic and edge cases.”
🤖 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/backend/src/yorkie/slides-tree.spec.ts` at line 76, In the "returns sensible defaults when reading an empty root" test in slides-tree.spec.ts, add an explicit assertion that result.guides equals an empty array to lock in the new canonical top-level shape; locate the test case (the one that currently checks defaults for an empty root) and add expect(result.guides).toEqual([]) alongside the existing default assertions so the test fails if guides is ever omitted or given a different default.packages/slides/test/store/memory.test.ts (1)
778-789: ⚡ Quick winThe “one undo step” assertion is currently non-discriminating.
Because state is
[]both before and afterundo(), this test can pass even if no undo entry was created. Assert stack behavior too (e.g.,canUndo()true after batch, then false after oneundo()).As per coding guidelines, “Write tests for critical business logic and edge cases.”
🤖 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/test/store/memory.test.ts` around lines 778 - 789, Test currently only checks final state array and can hide missing undo stack behavior; update the spec to assert undo stack behavior: after creating const store = new MemSlidesStore() and wrapping operations in store.batch(() => { const id = store.addGuide('x', 100); store.moveGuide(id, 200); store.removeGuide(id); }), assert store.canUndo() is true before calling store.undo(), then call store.undo() and assert store.canUndo() is false (and still assert read().guides remains []); use the MemSlidesStore, batch, addGuide, moveGuide, removeGuide, read, undo, and canUndo symbols to locate the test and add these extra expectations.
🤖 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 `@docs/design/slides/slides-ruler.md`:
- Around line 103-106: Update the design spec constants for the ruler thickness
from 20 px to 14 px: change the <ruler-corner> size to 14×14, set
<h-ruler-canvas> top/left offsets from top:0; left:20px; height:20px to top:0;
left:14px; height:14px, set <v-ruler-canvas> offsets from top:20px; left:0;
bottom:0; width:20px to top:14px; left:0; bottom:0; width:14px, and set
<canvas-pane> top/left offsets from top:20px; left:20px to top:14px; left:14px;
also apply the same 20→14px replacements for the other occurrences of these
elements mentioned (the later spec blocks for the same components).
In `@packages/frontend/src/app/slides/yorkie-slides-store.ts`:
- Around line 1476-1494: addGuide and moveGuide currently accept non-finite
positions (NaN/Infinity); add validation at the start of both functions to
reject non-finite values by using Number.isFinite(position) and throw a clear
Error when invalid. Update addGuide (before generateId/this.doc.update) and
moveGuide (before this.doc.update) to perform this check so invalid guide
geometry never gets written to the CRDT; keep existing calls to requireBatch(),
generateId(), and doc.update unchanged aside from the added guard.
In `@packages/slides/test/view/editor/snap.test.ts`:
- Around line 112-123: The test's intent is to verify that a user guide outranks
a closer element edge, but the fixture makes the guide closer (position 505
gives a 2px gap vs the edge's 3px). Update the guides fixture so the guide is
slightly farther than the edge but still within threshold (e.g., change guides =
[{ id: 'g1', axis: 'x', position: 507 }]) so the edge is the closer target (3px)
while the guide (4px) still qualifies, then run snapDelta(...) as before to
assert the guide wins; adjust the inline comments to reflect the new gaps if
present and keep references to snapDelta, guides, and others intact.
---
Duplicate comments:
In `@packages/frontend/src/app/slides/yorkie-slides-store.ts`:
- Around line 1476-1505: The guide mutations (addGuide, moveGuide, removeGuide)
are batched but undo/redo fails because replaceRoot doesn't restore
snapshot.guides; update the logic in replaceRoot to include snapshot.guides in
the restored state (ensure you copy/assign snapshot.guides onto the new root
object rather than ignoring it), preserve types for guides (Array<{id: string;
axis: 'x'|'y'; position: number}>) and use a defensive clone when applying
snapshot.guides so subsequent mutations don't share references with the
snapshot.
In `@packages/slides/src/store/memory.ts`:
- Around line 878-891: Validate that the position is a finite number in both
addGuide and moveGuide: before creating/adding the Guide in addGuide (function
addGuide) and before assigning guide.position in moveGuide (function moveGuide),
check Number.isFinite(position) and throw a clear error (e.g., "Invalid guide
position") if it isn't; keep the existing requireBatch() calls and error
behavior for missing guide in moveGuide.
---
Nitpick comments:
In `@packages/backend/src/yorkie/slides-tree.spec.ts`:
- Line 76: In the "returns sensible defaults when reading an empty root" test in
slides-tree.spec.ts, add an explicit assertion that result.guides equals an
empty array to lock in the new canonical top-level shape; locate the test case
(the one that currently checks defaults for an empty root) and add
expect(result.guides).toEqual([]) alongside the existing default assertions so
the test fails if guides is ever omitted or given a different default.
In `@packages/slides/test/store/memory.test.ts`:
- Around line 778-789: Test currently only checks final state array and can hide
missing undo stack behavior; update the spec to assert undo stack behavior:
after creating const store = new MemSlidesStore() and wrapping operations in
store.batch(() => { const id = store.addGuide('x', 100); store.moveGuide(id,
200); store.removeGuide(id); }), assert store.canUndo() is true before calling
store.undo(), then call store.undo() and assert store.canUndo() is false (and
still assert read().guides remains []); use the MemSlidesStore, batch, addGuide,
moveGuide, removeGuide, read, undo, and canUndo symbols to locate the test and
add these extra expectations.
🪄 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: c967d170-a7a6-45bc-9c8d-b78d073f8573
📒 Files selected for processing (40)
docs/design/README.mddocs/design/slides/slides-ruler.mddocs/tasks/README.mddocs/tasks/active/20260523-slides-ruler-todo.mdpackages/backend/src/yorkie/slides-tree.spec.tspackages/backend/src/yorkie/slides-tree.tspackages/docs/src/index.tspackages/docs/src/view/editor.tspackages/docs/src/view/ruler/index.tspackages/docs/src/view/ruler/tick-renderer.tspackages/docs/src/view/ruler/unit.tspackages/docs/test/view/ruler.test.tspackages/frontend/src/app/slides/slides-view.tsxpackages/frontend/src/app/slides/yorkie-slides-store.tspackages/frontend/src/types/slides-document.tspackages/frontend/src/types/users.tspackages/slides/src/import/pptx/index.tspackages/slides/src/index.tspackages/slides/src/model/migrate.tspackages/slides/src/model/presentation.tspackages/slides/src/node.tspackages/slides/src/store/memory.tspackages/slides/src/store/store.tspackages/slides/src/view/canvas/layout-preview.tspackages/slides/src/view/editor/editor.tspackages/slides/src/view/editor/notes-panel.tspackages/slides/src/view/editor/overlay.tspackages/slides/src/view/editor/ruler/index.tspackages/slides/src/view/editor/ruler/interactions.tspackages/slides/src/view/editor/ruler/ruler.tspackages/slides/src/view/editor/snap.tspackages/slides/test/store/memory.test.tspackages/slides/test/view/canvas/element-renderer.test.tspackages/slides/test/view/canvas/group-render.test.tspackages/slides/test/view/canvas/slide-renderer.test.tspackages/slides/test/view/canvas/thumbnail.test.tspackages/slides/test/view/editor/editor.test.tspackages/slides/test/view/editor/ruler/interactions.test.tspackages/slides/test/view/editor/ruler/ruler.test.tspackages/slides/test/view/editor/snap.test.ts
CodeRabbit review fixes (8): - migrate.ts: typed migrateGuide; synthesize id when missing; reject NaN/Infinity position (Critical — undo + render math break otherwise) - yorkie-slides-store: replaceRoot now restores snapshot.guides (Major — undo/redo of guide ops was silently dropping them) - memory.ts + yorkie store: assertFinitePosition on addGuide / moveGuide so non-finite values fail loudly instead of poisoning downstream snap / overlay math - notes-panel.ts: add a `:focus-visible` outline rule (WCAG 2.4.7 — the inline `outline: none` stripped the default ring; keyboard users now get a subtle 2-px inset ring) - overlay.ts: shift left/top by -0.5 px when thickening a snapped guide to 2 px so the line stays centred on the snap coordinate - ruler/interactions.ts: hitTestGuide now scans the full guide list and returns the closest within threshold (matches its docstring; prior implementation returned the first qualifying guide) - slides-view.tsx refitCanvas: re-clamp notesHeight against the live MAX_NOTES_H_RATIO cap so a localStorage value (or window resize) can't leave the panel exceeding the cap until next drag - slides-ruler.md: add `text` language tag to two fenced code blocks (markdownlint MD040) Feature — extend permanent guides into the ruler area: Permanent guide lines now extend past the slide bounds by GUIDE_EXTEND_PX (10 000) on each side so they visually connect into the H/V rulers, matching the Figma / Sketch convention. canvasArea gains `overflow: hidden` to clip the excess at the frame edge so lines don't leak into the speaker-notes panel. canvasWrap moves to `z-index: 2` and the ruler canvases drop to `z-index: 1` so the guide divs paint above the ruler ticks at their crossing points (corner stays at z-index 3; no guide can cross it because guides always sit at slide-x ≥ 0, which maps well to the right of the corner's footprint after frame padding + centring). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
- design doc: sync the 20-px ruler references to the shipped 14-px slim ruler (DOM tree snippet at lines 100-110 + nearby prose + risks table). Spec drift would otherwise mislead anyone reading slides-ruler.md after the slim-ruler commit. - snap.test priority case: the fixture had the guide closer (2 px) than the edge (3 px), so the test trivially passed even without the priority rule. Move the guide to position 507 so the edge is the closer candidate (3 px) and the guide (4 px) only wins via priority — the rule the test actually exists to pin. Skipped: the duplicate "reject non-finite guide positions in Yorkie mutators" comment — `assertFiniteGuidePosition` already guards both `addGuide` and `moveGuide` on the YorkieSlidesStore (added in the previous review commit at line 1512); CodeRabbit appears to have re-flagged it against a pre-fix snapshot. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
There was a problem hiding this comment.
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/notes-panel.ts (1)
61-61:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInline
outline: nonesuppresses the injected:focus-visiblering
ta.style.outline = 'none'sets theoutlineproperty inline, which outranks the stylesheet rule injected byensureNotesPanelStyles(); the.wfb-slides-notes-ta:focus-visibleoutline won’t reliably show for keyboard users.♿ Minimal fix
- ta.style.outline = 'none'; + // Outline behavior is controlled in ensureNotesPanelStyles().style.textContent = + '.wfb-slides-notes-ta:focus { outline: none; }' + '.wfb-slides-notes-ta:focus-visible {' + ' outline: 2px solid var(--ring, `#3a7`);' + ' outline-offset: -2px;' + ' border-radius: 2px;' + '}';🤖 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/notes-panel.ts` at line 61, The inline assignment ta.style.outline = 'none' overrides the stylesheet's .wfb-slides-notes-ta:focus-visible ring, so remove that inline outline setting (delete the ta.style.outline = 'none' line) and rely on ensureNotesPanelStyles() / the .wfb-slides-notes-ta class to provide the focus-visible outline for keyboard users; if you must reset outline programmatically, remove the inline property with ta.style.removeProperty('outline') instead of setting it to 'none'.
🧹 Nitpick comments (1)
packages/slides/src/model/migrate.ts (1)
47-53: ⚡ Quick winPrefer
unknown+ narrowing instead ofanyinmigrateGuide
migrateGuideis the migration boundary for arbitrary document data; usingunknownpreserves the existing runtime behavior while keeping the input type-safe.♻️ Suggested refactor
-function migrateGuide(g: any): Guide { - const id = typeof g?.id === 'string' && g.id.length > 0 ? g.id : generateId(); - const axis: GuideAxis = g?.axis === 'y' ? 'y' : 'x'; - const rawPos = g?.position; +function migrateGuide(g: unknown): Guide { + const raw = (g ?? {}) as { + id?: unknown; + axis?: unknown; + position?: unknown; + }; + const id = typeof raw.id === 'string' && raw.id.length > 0 ? raw.id : generateId(); + const axis: GuideAxis = raw.axis === 'y' ? 'y' : 'x'; + const rawPos = raw.position; const position = typeof rawPos === 'number' && Number.isFinite(rawPos) ? rawPos : 0; return { id, axis, position }; }🤖 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/model/migrate.ts` around lines 47 - 53, Change the parameter type of migrateGuide from any to unknown and keep the existing runtime narrowing checks (typeof g?.id === 'string', g?.axis === 'y', typeof rawPos === 'number' && Number.isFinite(rawPos)) so the function remains a safe migration boundary; update the signature migrateGuide(g: unknown): Guide and refer to g only after those guards (no unchecked property access or type assertions) so the current logic that computes id, axis, and position is preserved while using unknown for type safety.
🤖 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.
Outside diff comments:
In `@packages/slides/src/view/editor/notes-panel.ts`:
- Line 61: The inline assignment ta.style.outline = 'none' overrides the
stylesheet's .wfb-slides-notes-ta:focus-visible ring, so remove that inline
outline setting (delete the ta.style.outline = 'none' line) and rely on
ensureNotesPanelStyles() / the .wfb-slides-notes-ta class to provide the
focus-visible outline for keyboard users; if you must reset outline
programmatically, remove the inline property with
ta.style.removeProperty('outline') instead of setting it to 'none'.
---
Nitpick comments:
In `@packages/slides/src/model/migrate.ts`:
- Around line 47-53: Change the parameter type of migrateGuide from any to
unknown and keep the existing runtime narrowing checks (typeof g?.id ===
'string', g?.axis === 'y', typeof rawPos === 'number' &&
Number.isFinite(rawPos)) so the function remains a safe migration boundary;
update the signature migrateGuide(g: unknown): Guide and refer to g only after
those guards (no unchecked property access or type assertions) so the current
logic that computes id, axis, and position is preserved while using unknown for
type safety.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89b9f4ad-e2c7-4a70-aa1b-957a802dc882
📒 Files selected for processing (9)
docs/design/slides/slides-ruler.mdpackages/frontend/src/app/slides/slides-view.tsxpackages/frontend/src/app/slides/yorkie-slides-store.tspackages/slides/src/model/migrate.tspackages/slides/src/store/memory.tspackages/slides/src/view/editor/notes-panel.tspackages/slides/src/view/editor/overlay.tspackages/slides/src/view/editor/ruler/interactions.tspackages/slides/test/view/editor/snap.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/design/slides/slides-ruler.md
2 tasks
hackerwins
added a commit
that referenced
this pull request
May 24, 2026
The root "build" script never built @wafflebase/docs and ran every package build concurrently with no ordering. Since cross-package type resolution reads each dependency's built dist/*.d.ts (gitignored, often stale) rather than its source, consumers compiled against outdated declarations: slides failed on missing docs ruler exports and backend failed on the slides Guide/guides symbols added in #285. Build in topological order instead: (docs, sheets) -> slides -> (frontend, backend, cli), so every package compiles against freshly built dependency declarations. Verified from a clean dist state.
3 tasks
hackerwins
added a commit
that referenced
this pull request
May 24, 2026
These tasks' features merged to main (PRs #267 #271 #277 #284 #285) but their plan checkboxes were never ticked, so the todo files lingered in tasks/active. Added a status note citing each merge commit, marked the checkboxes complete, and ran tasks:archive to move them under archive/2026/05 with regenerated indexes. Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
7 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
@wafflebase/docs(slides extractstickHeights/labelFontSize/labelInsetparams; docs behaviour preserved bit-for-bit).Design:
docs/design/slides/slides-ruler.md· Plan:docs/tasks/active/20260523-slides-ruler-todo.mdWhat landed (13 commits across 6 phases + UX polish)
SlidesRulercontroller, H/V canvases, 144 dpi, zoom-aware density.Guidetype + Yorkie schema + Mem/Yorkie store APIs + passive overlay paint.slide-center > guide > edge; snapped guide is thickened + deepened.Test plan
col-resize/row-resize) → repositions on mouseup.slide-centersnap still wins over a guide within the same threshold; guide still wins over a closer element edge.wfb-slides-notes-height); left thumbnail width persists (wfb-slides-left-width).pnpm verify:fastpasses (1273 slides tests + 792 docs tests + backend / frontend gates).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation