Home Original page

refactor: improve types usage in PageBreaksMixin by tbouffard · Pull Request #956 · maxGraph/maxGraph

Type safety refinement for PageBreaksMixin introducing explicit Shape typing for internal drawPageBreaks function and public horizontalPageBreaks/verticalPageBreaks properties, replacing generic any types with Shape[] | null and adding null guards in control flow.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 799f385 and 05f10ce.

📒 Files selected for processing (2)
  • packages/core/src/view/mixins/PageBreaksMixin.ts (2 hunks)
  • packages/core/src/view/mixins/PageBreaksMixin.type.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings 
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 857
File: packages/core/src/view/plugins/SelectionHandler.ts:1081-1092
Timestamp: 2025-06-26T13:27:19.012Z
Learning: User tbouffard prefers to keep PRs focused on their main objective and defer implementation improvements or logic fixes to future dedicated PRs. For maxGraph, refactoring PRs like Dictionary→Map migration should not include unrelated logic improvements even when valid issues are identified in the migrated code.

Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 776
File: packages/core/src/view/AbstractGraph.ts:492-495
Timestamp: 2025-04-24T12:34:56.726Z
Learning: PR #776 focuses on refactoring to introduce AbstractGraph as a base class and moving code from Graph to AbstractGraph. Type improvements and other refinements should be addressed in future PRs to keep this one focused on the core architectural change.

Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 849
File: packages/html/stories/DragSource.stories.js:98-101
Timestamp: 2025-06-13T07:48:10.300Z
Learning: User tbouffard prefers answers in English; avoid switching to other languages in future replies.
📚 Learning: 2025-04-28T08:24:39.831Z 
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.

Applied to files:

  • packages/core/src/view/mixins/PageBreaksMixin.type.ts
  • packages/core/src/view/mixins/PageBreaksMixin.ts
📚 Learning: 2025-04-28T08:24:39.831Z 
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the './builtin-style-elements' file as a namespace export ('export * as Perimeter'), making this the correct import location for accessing the Perimeter namespace.

Applied to files:

  • packages/core/src/view/mixins/PageBreaksMixin.type.ts
  • packages/core/src/view/mixins/PageBreaksMixin.ts
🧬 Code graph analysis (1)
packages/core/src/view/mixins/PageBreaksMixin.ts (1)
packages/html/stashed/grapheditor/www/js/Editor.js (4)
  • drawPageBreaks (2313-2350)
  • horizontalCount (2300-2300)
  • verticalCount (2301-2301)
  • pageBreak (2333-2333)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build (macos-14)
  • GitHub Check: build (ubuntu-24.04)
  • GitHub Check: build (windows-2022)
🔇 Additional comments (6)
packages/core/src/view/mixins/PageBreaksMixin.type.ts (2)

17-17: LGTM! Shape import added for type declarations.

This import is necessary to support the improved type declarations for horizontalPageBreaks and verticalPageBreaks.

22-22: Excellent type safety improvement!

Replacing any[] | null with Shape[] | null for both horizontalPageBreaks and verticalPageBreaks provides stronger type guarantees and aligns perfectly with the PR objective.

Also applies to: 25-25

packages/core/src/view/mixins/PageBreaksMixin.ts (4)

21-21: LGTM! Shape import added for type safety.

This import enables the improved type signature for the drawPageBreaks function.

81-84: Good defensive programming with typed signature.

The function signature now explicitly accepts Shape[] | null, and the null guard provides a clean early return. This improves both type safety and readability.

106-126: LGTM! Page break lifecycle management is well-typed.

The logic correctly handles:

  • Updating existing breaks by modifying points and calling redraw (lines 107-108)
  • Creating new PolylineShape instances with proper initialization (lines 110-117)
  • Cleaning up excess breaks with destroy and splice (lines 121-125)

The explicit Shape typing throughout makes the code safer and more maintainable.

88-104: Calculation logic is consistent with original implementation.

Verification of the git history confirms the point calculation i * bounds.height and i * bounds.width has remained unchanged. The most recent refactor (commit 05f10ce) preserved this calculation exactly as-is—only restructuring the surrounding control flow. No (i + 1) offset pattern exists in the codebase, and the first page break placement at the origin (i=0) matches the original implementation.

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.