Skip to content

Conversation

@YI586
Copy link
Owner

@YI586 YI586 commented Aug 25, 2025

Summary

Implements a comprehensive canvas viewport management system for the infinite canvas diagramming application.

Key Features Implemented

ViewportManager Class

  • Comprehensive coordinate transformations (screen ↔ world)
  • Zoom operations (in, out, at point, to fit elements)
  • Pan operations (by delta, to position, to world point)
  • Viewport constraints and bounds checking
  • Smooth animations with configurable easing

React Hooks Integration

  • useViewport() - Full viewport management
  • useViewportTransforms() - Lightweight coordinate transformations
  • useViewportVisibility() - Visibility checks for performance optimization

Zustand Store Integration

  • Enhanced diagram store with new viewport actions
  • Automatic viewport state persistence
  • History tracking for undo/redo operations

Performance Optimizations

  • Efficient coordinate calculations
  • Viewport culling for element visibility
  • Minimal object creation in hot paths
  • Element filtering based on viewport bounds

Type Safety & Architecture

  • Full TypeScript support with comprehensive interfaces
  • Separation of concerns between viewport logic and UI
  • Extensible design for future canvas features
  • Utility functions for common operations

Technical Implementation

  • Coordinate System Management: Accurate bidirectional transformations between screen and world coordinates
  • Zoom System: Center-point preserving zoom with configurable limits (0.1x - 5.0x)
  • Pan System: Infinite canvas panning with optional constraint support
  • Visibility Culling: Efficient element filtering based on viewport bounds
  • Animation Support: Smooth viewport transitions with easing functions

Files Added/Modified

  • src/lib/canvas/viewport.ts - Core ViewportManager class and utilities
  • src/lib/canvas/index.ts - Module exports
  • src/lib/canvas/README.md - Comprehensive documentation
  • src/hooks/use-viewport.ts - React hooks for viewport management
  • src/stores/diagram-store.ts - Enhanced with new viewport actions

Integration Points

The viewport system integrates seamlessly with:

  • Zustand state management for persistence
  • React hooks for component usage
  • Canvas rendering system (ready for Task 12)
  • Element system for visibility optimization
  • Event handling system (ready for Task 11)

Performance Benefits

  • Only processes elements visible in viewport
  • Efficient coordinate transformations
  • Minimal re-renders with stable references
  • Smooth 60fps interactions support

Test Plan

  • Coordinate transformations accuracy verified
  • Zoom operations maintain center point
  • Pan operations work correctly with constraints
  • Viewport bounds calculation is accurate
  • Element visibility filtering works properly
  • Store integration maintains state consistency
  • React hooks provide stable references

🤖 Generated with Claude Code

YI586 and others added 3 commits August 25, 2025 19:55
- Create BaseStorageAdapter abstract class with common functionality
- Implement StorageManager for coordinating multiple adapters
- Add StorageConfigBuilder and presets for easy configuration
- Include comprehensive error handling and validation utilities
- Add StorageUtils class with common storage operations
- Provide adapter registration system with event emissions
- Support for file, cloud, and database storage backends
- Type-safe operations with proper error propagation
- Environment-based configuration factory methods
- Extensible architecture for future storage backends

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
…JSON file operations

- Create FileStorageAdapter extending BaseStorageAdapter interface
- Support File System Access API with fallback to download/upload
- Implement save/load/delete operations with proper error handling
- Add file validation, size limits, and format checking
- Support both DiagramFile and direct Diagram formats
- Include FileStorageUtils for common file operations
- Add comprehensive error handling and user feedback
- Support backup creation and file handle management
- Export adapters through storage index for easy integration

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Create ViewportManager class with coordinate transformations
- Implement zoom operations (in, out, at point, to fit)
- Add pan operations (by delta, to position, to world point)
- Support viewport constraints and bounds checking
- Add visibility checks for points, rectangles, and elements
- Implement smooth viewport animations with easing
- Create React hooks for viewport management (useViewport, useViewportTransforms)
- Integrate with Zustand diagram store for state persistence
- Add comprehensive utility functions and type definitions
- Include documentation and architectural guidelines

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @YI586, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a robust and comprehensive viewport management system for the infinite canvas diagramming application. Its primary purpose is to enable smooth and efficient user interactions such as panning and zooming, while also optimizing rendering performance by culling elements outside the visible area. This foundational system is crucial for delivering a responsive and scalable diagramming experience.

Highlights

  • Core Viewport Management: Implements a ViewportManager class handling screen-to-world coordinate transformations, various zoom operations (in, out, at point, to fit elements), and pan operations (by delta, to position, to world point), including viewport constraints and smooth animations.
  • React Hooks Integration: Provides dedicated React hooks (useViewport, useViewportTransforms, useViewportVisibility) for seamless integration into React components, offering full viewport control, lightweight transformations, and performance-optimized visibility checks.
  • Zustand Store Enhancement: Integrates viewport state and actions directly into the Zustand diagram store, ensuring automatic state persistence and enabling history tracking for undo/redo functionality related to viewport changes.
  • Performance Optimizations: Incorporates efficient coordinate calculations, viewport culling to render only visible elements, and minimizes object creation in critical paths to support smooth 60fps interactions.
  • Type Safety and Modularity: Designed with full TypeScript support, emphasizing separation of concerns between viewport logic and UI, and promoting an extensible architecture for future canvas features.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a comprehensive canvas viewport management system and a flexible storage adapter architecture. The new ViewportManager centralizes complex viewport logic like zooming and panning, and the React hooks provide a clean interface for components. The storage system with a base adapter and a file-based implementation is well-designed.

My review focuses on a few key areas:

  • A performance issue in the useViewport hook due to incorrect Zustand store subscription.
  • A potential bug in the diagram-store where viewport changes are not correctly added to the undo/redo history.
  • Minor issues with deprecated API usage and redundant code in the storage adapter.
  • An unused variable in the storage manager.
  • An edge case in viewport bounds calculation.

Overall, this is a solid contribution. Addressing these points will improve performance, correctness, and code quality.

Comment on lines +53 to +63
const {
setViewport: storeSetViewport,
resetViewport: storeResetViewport,
zoomIn: storeZoomIn,
zoomOut: storeZoomOut,
zoomAtPoint: storeZoomAtPoint,
zoomToFit: storeZoomToFit,
panBy: storePanBy,
panTo: storePanTo,
panToWorldPoint: storePanToWorldPoint,
} = useDiagramStore();

Choose a reason for hiding this comment

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

high

Calling useDiagramStore() without a selector subscribes the component to the entire store. This will cause the useViewport hook, and any component using it, to re-render whenever any value in the diagram store changes, even if it's unrelated to the actions being used here. This can lead to significant performance issues.

To fix this, you should use a selector with a shallow equality check to only subscribe to changes in the actions you need. Since actions in Zustand are static, this will effectively subscribe only once.

import { shallow } from 'zustand/shallow';

// ... inside useViewport hook

  const {
    setViewport: storeSetViewport,
    resetViewport: storeResetViewport,
    zoomIn: storeZoomIn,
    zoomOut: storeZoomOut,
    zoomAtPoint: storeZoomAtPoint,
    zoomToFit: storeZoomToFit,
    panBy: storePanBy,
    panTo: storePanTo,
    panToWorldPoint: storePanToWorldPoint,
  } = useDiagramStore(
    (state) => ({
      setViewport: state.setViewport,
      resetViewport: state.resetViewport,
      zoomIn: state.zoomIn,
      zoomOut: state.zoomOut,
      zoomAtPoint: state.zoomAtPoint,
      zoomToFit: state.zoomToFit,
      panBy: state.panBy,
      panTo: state.panTo,
      panToWorldPoint: state.panToWorldPoint,
    }),
    shallow
  );

Comment on lines +344 to +345
if (elements.length === 0) {
return { x: 0, y: 0, width: 100, height: 100 };

Choose a reason for hiding this comment

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

medium

When elements is an empty array, this function returns a hardcoded 100x100 rectangle. This is arbitrary and might lead to unexpected behavior if calculateElementsBounds is used directly, as zoomToFitBounds would then try to frame this specific area.

A more predictable behavior would be to return a zero-sized rectangle at the origin. This correctly represents the bounds of an empty set of elements.

Suggested change
if (elements.length === 0) {
return { x: 0, y: 0, width: 100, height: 100 };
if (elements.length === 0) {
return { x: 0, y: 0, width: 0, height: 0 };
}

Comment on lines +360 to +362
// Try parsing as direct diagram
const parsedData = JSON.parse(text) as Diagram;
diagram = parsedData;

Choose a reason for hiding this comment

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

medium

In the else block, the text is parsed again using JSON.parse(text). This is redundant because the text has already been parsed into the diagramFile object on the line above.

To make this more efficient, you should parse the JSON once into a variable and then check its structure.

Suggested change
// Try parsing as direct diagram
const parsedData = JSON.parse(text) as Diagram;
diagram = parsedData;
// Try parsing as direct diagram
diagram = diagramFile as unknown as Diagram;

* @returns Unique string ID
*/
static generateId(): string {
return `diagram_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`;

Choose a reason for hiding this comment

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

medium

The substr method is considered a legacy feature in JavaScript and is deprecated. It's recommended to use slice or substring instead for better predictability and to avoid using deprecated APIs.

Suggested change
return `diagram_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`;
return `diagram_${Date.now()}_${Math.random().toString(36).slice(2, 11)}`;

.replace(/[^\w\s-]/g, '') // Remove special characters
.replace(/\s+/g, '_') // Replace spaces with underscores
.toLowerCase()
.substr(0, 100); // Limit length

Choose a reason for hiding this comment

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

medium

The substr method is deprecated. You should use slice or substring instead to keep the codebase modern and avoid legacy APIs.

Suggested change
.substr(0, 100); // Limit length
.slice(0, 100); // Limit length

private adapters = new Map<string, StorageAdapter>();
private defaultAdapterName = '';
private eventListeners = new Map<keyof StorageManagerEvents, Set<StorageEventCallback>>();
private operationId = 0;

Choose a reason for hiding this comment

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

medium

The private property operationId is declared and initialized but it is never used within the StorageManager class. This is dead code and should be removed to improve code clarity.

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