Skip to content

feat(design-canvas): native fit-on-mount + onReady#58

Merged
drewstone merged 1 commit into
mainfrom
feat/canvas-fit-on-mount
Jun 14, 2026
Merged

feat(design-canvas): native fit-on-mount + onReady#58
drewstone merged 1 commit into
mainfrom
feat/canvas-fit-on-mount

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Why

The design-canvas mounts at zoom 1 / pan 0 with no auto-fit — a default artboard sits parked off the top-left of the dark canvas, so a fresh design reads as "broken / nothing there." The only fit trigger today is a window f keydown + the Fit button, which forced consumers (gtm-agent) to dispatch a synthetic keypress as a bridge.

What

  • New opt-in fitOnMount?: boolean (default true) on DesignCanvasProps: fits the active page to the viewport once, on the first non-zero measurement, using the same zoomPanMath.fitPage(...) + stack.setView(...) the f key already calls.
  • New onReady?() callback — fires once after that first real measurement (after the initial fit, or immediately when fitOnMount is false).
  • One-shot via hasFittedRef: a page switch re-subscribes the ResizeObserver but does not re-fit, preserving the user's zoom/pan. The existing f-key / Fit-button path is untouched for manual re-fit.
  • Guards (width/height > 0, activePage.width/height > 0) prevent fitPage throwing on zero dims.

Plumbed through both paths: the custom-integrator renderWorkspace(ctx) render-prop (DesignCanvas.tsx) and the batteries-included DesignCanvasEditor.tsx.

Consumer impact

fitOnMount defaults to true, so consumers just delete their fit bridge — no prop change needed. Pass fitOnMount={false} + use onReady only when restoring a saved viewport.

Verified

pnpm build (tsup) clean; props present in built dist/.../index.d.ts; tsc clean; 439/439 design-canvas tests pass. Version 0.16.0 (additive over published 0.15.0). Merges cleanly into main.

Add an opt-in fitOnMount prop (default true) so consumers no longer need a
synthetic-keypress bridge to fit the active page on mount. On the first
non-zero ResizeObserver measurement, the workspace fits the active page to
the viewport once via the same zoomPanMath.fitPage + stack.setView the 'f'
key and Fit button use. A hasFittedRef guard keeps it single-fire across the
effect's re-subscriptions, so page switches preserve the user's zoom+pan.

onReady fires once after that first real measurement (after the fit is
applied, or skipped when fitOnMount is false). Both props thread through
DesignCanvasProps -> renderWorkspace ctx -> WorkspaceView, and through the
batteries-included DesignCanvasEditor default path.

Version 0.16.0 (additive over the published 0.15.0).

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved PR — d1eba407

Blanket team auto-approval is enabled for this reviewer service.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: blanket_auto_approve · 2026-06-14T21:36:45Z

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Value Audit — sound

Verdict sound
Concerns 0 (none)
Heuristic 0.0s
Duplication 0.0s
Interrogation 123.0s (2 bridge agents)
Total 123.0s

💰 Value — sound

Adds a sensible default auto-fit-on-mount to the design-canvas using existing zoom/pan primitives, plus an onReady hook for viewport restore, wired through both the render-prop and editor paths.

  • What it does: The change adds two opt-in props to DesignCanvasProps (src/design-canvas-react/contracts.ts:163-166): fitOnMount?: boolean (default true) and onReady?(): void. In WorkspaceView (src/design-canvas-react/components/Workspace.tsx:150-197), the existing ResizeObserver callback now performs a one-shot fit the first time the container reports non-zero dimensions: if fitOnMount is true
  • Goals it achieves: The change fixes the fresh-mount UX where the artboard sat at zoom 1 / pan 0, often appearing off-canvas or empty. It removes the need for consumers to dispatch synthetic keypresses to trigger a fit, and it gives consumers that restore saved viewports a deterministic onReady signal so they can apply a saved view after the initial measurement without racing the default fit.
  • Assessment: Good. The implementation is coherent and additive: it reuses the existing fitPage math (src/design-canvas-react/engine/zoom-pan.ts:61-90) and the existing stack.setView view-state mutation, keeps the manual fit path intact, guards against zero dimensions, and provides an explicit opt-out. It follows the established pattern of container measurement via ResizeObserver already used by `Worksp
  • Better / existing approach: none — this is the right approach. I searched the repo for existing auto-fit / on-mount fit abstractions (fitOnMount, autoFit, fitTo, initialFit, centerOnMount) and found no existing equivalent in src/design-canvas-react or elsewhere. The only fit logic is the existing manual fitPage math and the onFitRef callback used by the f key / Fit button; this change extends that path rath

🎯 Usefulness — sound

Auto-fit on mount plus an onReady hook is a coherent, additive fix to a real UX gap; it reuses the existing fit math, exposes props through the public surface, and is wired through both the raw chrome and batteries-included editor paths.

  • Integration: Correct and reachable. fitOnMount/onReady are declared on DesignCanvasProps (contracts.ts:163-166), forwarded by DesignCanvas into renderWorkspace (DesignCanvas.tsx:616-617), consumed by DesignCanvasEditor and passed into WorkspaceView (DesignCanvasEditor.tsx:217-218, Workspace.tsx:150-151), and exported publicly via src/design-canvas-react/index.ts re-exporting ./contracts. No c
  • Fit with existing patterns: It extends the established manual-fit pattern rather than competing with it. The same zoomPanMath.fitPage(activePage, {width,height}) + stack.setView(view) path used by the f key / Fit button (Workspace.tsx:207-211, DesignCanvas.tsx:466-469) is reused; only the trigger changes from user action to a one-shot ResizeObserver.
  • Real-world viability: Holds up for the stated cases. Guards prevent fitPage from throwing on zero viewport or page dims (Workspace.tsx:187-191), and hasFittedRef keeps it one-shot across page switches and effect re-subscriptions. The dependency array includes activePage, so the observer reconnects on document mutations; this is acknowledged in the PR as page-switch behavior and does not affect correctness, only c

No concerns — sound change, no better or existing approach found. ✅


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260614T214035Z

@tangletools

Copy link
Copy Markdown

✅ No Blockers — d1eba407

Readiness 79/100 · Confidence 70/100 · 6 findings (2 medium, 4 low)

deepseek glm aggregate
Readiness 79 79 79
Confidence 70 70 70
Correctness 79 79 79
Security 79 79 79
Testing 79 79 79
Architecture 79 79 79

Full multi-shot audit completed 2/2 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 2/2 planned shots over 5 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM Standalone Workspace wrapper silently drops fitOnMount and onReady props — src/design-canvas-react/components/Workspace.tsx

Lines 870-914: export function Workspace(props: DesignCanvasProps) receives the new fitOnMount and onReady props (they are now on DesignCanvasProps via contracts.ts:163-166), but the JSX at lines 904-913 does not forward them to . Impact: any consumer of the standalone Workspace wrapper passing fitOnMount={false} (e.g. to restore a saved viewport) will still get fit-on-mount behavior because WorkspaceView defaults fitOnMount to true ([line 150](https://github.com/tan

🟠 MEDIUM fitOnMount/onReady not forwarded by standalone Workspace wrapper — src/design-canvas-react/components/Workspace.tsx

The standalone Workspace component (line 870, renders WorkspaceView at line 903) accepts DesignCanvasProps which now includes fitOnMount and onReady, but does not pass them to the child WorkspaceView. This means consumers using bare Workspace (not DesignCanvasEditor) will see fitOnMount={false} silently ignored — WorkspaceView will always use its default of true. Fix: add fitOnMount={props.fitOnMount} and onReady={props.onReady} to the WorkspaceView JSX at [lines 904-913](h

🟡 LOW Redundant prop re-declaration in DesignCanvasFullProps — src/design-canvas-react/components/DesignCanvas.tsx

DesignCanvasFullProps extends DesignCanvasProps (which already declares fitOnMount?: boolean and onReady?(): void) and then re-declares both (lines 85-88) identically. TypeScript accepts this with compatible types, and the duplicate declarations also appear inside renderWorkspace's context parameter (lines 83-88). Neither causes a type error or runtime issue, but they are dead weight — removing them would make the source of truth clearer. N.B.: the duplicate inside the context in

🟡 LOW No test coverage for fitOnMount/onReady behavior — src/design-canvas-react/components/Workspace.tsx

The new useLayoutEffect ResizeObserver logic (lines 176-197) that gates fit-on-mount and fires onReady has zero test coverage. A grep for fitOnMount or onReady across the entire tests/ directory returns no results. The hasFittedRef guard (preventing re-fit on page switch) and onReady single-fire semantics are undocumented-in-code invariants that a test would catch if broken. Consider adding a render test that verifies: (1) with fitOnMount=true, the WorkspaceView calls fitPage after first ResizeObserver measurement; (2) with fitOnMount=false, it does not; (3) onReady fires exactly once after the first non-zero measurement.

🟡 LOW No tests for new fit-on-mount lifecycle behavior — src/design-canvas-react/components/Workspace.tsx

The PR adds non-trivial React lifecycle logic — one-shot fit via hasFittedRef, onReady callback semantics, interaction with page switches (activePage identity change should NOT re-fit), and the fitOnMount=false skip path — but tests/design-canvas/ has zero coverage for any of it. Existing tests only cover engine-level fitPage and setView. A jsdom + ResizeObserver mock test verifying: (a) fit fires once on first measurement, (b) does not re-fire on page switch, (c) onReady fires even when fitOnMount=false, (d) fit is skipped when fitOnMount=false — would protect the hasFittedRef invariant from regression.

🟡 LOW onReady in useLayoutEffect deps causes unnecessary ResizeObserver churn with unstable callbacks — src/design-canvas-react/components/Workspace.tsx

Line 197: deps changed from [] to [activePage, fitOnMount, onReady, stack, zoomPanMath]. If a parent passes an inline onReady (common React pattern — no useCallback), the effect tears down and recreates the ResizeObserver on every parent re-render. Each new observer fires immediately on observe(), calling setContainerSize({width, height}) with a fresh object even when dimensions are unchanged, triggering an avoidable re-render. The hasFittedRef guard prevents re-fitting so this is not a correctness bug, but it degrades performance. Recommended fix: store onReady and fitOnMount in refs (updated on every render) and read from refs inside the


tangletools · 2026-06-14T21:45:36Z · trace

@drewstone drewstone merged commit 2b9c38b into main Jun 14, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants