feat: whiteboard history and auto-save#40
Conversation
cosarah
left a comment
cosarah
left a comment
There was a problem hiding this comment.
Code Review
Overall the history/auto-save feature is well-designed — Zustand store is clean, snapshot logic covers the main scenarios, and i18n is complete. A few issues to address:
1. PR Scope
This PR bundles two independent features:
Recommend splitting into two PRs for easier review and rollback.
2. Pan doesn't track 1:1 with mouse at different zoom levels
whiteboard-canvas.tsx:237-239
const effectiveScale = containerScale * autoFitTransform.scale * viewZoom;
setPanX(panStartRef.current.panX + dx / effectiveScale);The CSS transform is translate(tx, ty) scale(s) with origin 0 0. Since translate is applied before scale, panX changes appear on screen as panX_delta * containerScale — the content scale (autoFitTransform.scale * viewZoom) does not multiply the translation.
Dividing by the extra autoFitTransform.scale * viewZoom makes dragging feel sluggish when zoomed in and too fast when zoomed out. For Figma/Google Maps-style 1:1 tracking (content under cursor follows cursor exactly), the divisor should be just containerScale:
setPanX(panStartRef.current.panX + dx / containerScale);3. "Reset View" button swallowed by pan
whiteboard-canvas.tsx:366-376
The reset button is a child of the canvas div that has onPointerDown. When clicking the button, pointerdown fires first on the canvas (starts panning), then the button's onClick resets the view, but on pointerup the pan state interferes. This makes it feel like the button requires a double-click.
Fix: stop pointer propagation on the button, or check the event target in handlePointerDown:
<motion.button
onPointerDown={(e) => e.stopPropagation()}
onClick={(e) => {
e.stopPropagation();
handleDoubleClick(e as unknown as React.MouseEvent);
}}
...
>4. History deduplication needed
lib/store/whiteboard-history.ts:47-65
Repeatedly clear → restore → clear creates duplicate snapshots of identical content, wasting the 20-snapshot limit. Add a dedup check before pushing:
pushSnapshot: (elements, label) => {
if (!elements || elements.length === 0) return;
const { snapshots, maxSnapshots } = get();
// Skip if identical to the latest snapshot
const latest = snapshots[snapshots.length - 1];
if (latest) {
const latestIds = latest.elements.map((e) => e.id).join(',');
const newIds = elements.map((e) => e.id).join(',');
if (latestIds === newIds) return;
}
// ... rest of push logic
}5. isRestoring relies on fragile timing
whiteboard-history.tsx:73-75
setTimeout(() => {
useWhiteboardHistoryStore.getState().setRestoring(false);
}, 100);The 100ms magic number works only because the auto-snapshot debounce is 2s. A more robust approach: record the restored elementsKey in the store, and skip snapshotting when the current key matches:
// In store
restoredKey: null as string | null,
setRestoredKey: (key: string | null) => set({ restoredKey: key }),
// In auto-snapshot effect
if (elementsKey === restoredKey) {
useWhiteboardHistoryStore.getState().setRestoredKey(null);
// skip snapshot
}This eliminates the timing dependency entirely.
6. useEffect dependency issue
whiteboard-canvas.tsx:132
}, [elementsKey, elements]);elementsKey is derived from elements, but if the store returns a new array reference with the same IDs, the effect re-runs unnecessarily. Depend only on elementsKey and read elements from a ref inside the effect.
7. Minor
whiteboard-canvas.tsx:374:t('whiteboard.resetView') ?? 'Reset View'— the i18n key exists in both locales,?? 'Reset View'is dead code.whiteboard-canvas.tsx:369:e as unknown as React.MouseEvent— unnecessary double cast,motion.button'sonClickalready providesReact.MouseEvent.whiteboard-canvas.tsx:102:elements.map((e) => e.id).join(',')runs every render — wrap inuseMemo.
YizukiAme
force-pushed
the
feat/whiteboard-history
branch
from
5eda039 to
72df8fb
Compare
PR Split Complete ✅This PR has been split into two independent features as requested: Feature A: Pan/Zoom/Auto-fit
Feature B: History & Auto-save
IndependenceBoth features are completely independent:
RecommendationSuggest reviewing Feature A (pan/zoom) first as it's simpler and addresses a more fundamental UX issue (content overflow). |
…afety - Extract duplicated elementFingerprint to lib/utils/element-fingerprint.ts - Replace throw error with toast.error in handleRestore (prevents app crash) - Add useMemo for elementsKey in whiteboard-canvas (perf optimization) - Fix unsafe (e as any).height cast with proper type narrowing
- Move `elementsRef.current = elements` into useEffect to fix react-hooks/refs lint error (cannot update ref during render) - Wrap `elements` in useMemo to stabilise the reference and fix react-hooks/exhaustive-deps warning on useMemo dependencies
2 participants
What
Add whiteboard history and auto-save feature — users can browse and restore previous whiteboard states.
Closes #32
Changes
lib/store/whiteboard-history.tscomponents/whiteboard/whiteboard-history.tsxcomponents/whiteboard/index.tsxcomponents/whiteboard/whiteboard-canvas.tsxisRestoringguardlib/i18n/stage.tsHow It Works
isRestoringflag to prevent the restore itself from creating a new snapshotDesign Decisions
Testing