refactor: improve types usage in PageBreaksMixin #956

tbouffard · 2025-11-24T15:11:10Z

Remove some any and use Shape instead.

Summary by CodeRabbit

Refactor
- Enhanced type safety and consistency in page break handling with more explicit typing throughout the codebase.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Remove some any and use Shape instead.

coderabbitai · 2025-11-24T15:11:47Z

Walkthrough

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.

Changes

Cohort / File(s)	Change Summary
PageBreaksMixin type safety `packages/core/src/view/mixins/PageBreaksMixin.ts`	Imported Shape type; updated drawPageBreaks function signature to accept Shape[] \| null instead of any; added null/undefined guard with early return; refined internal handling of horizontalPageBreaks and verticalPageBreaks as typed Shape arrays.
PageBreaksMixin type definitions `packages/core/src/view/mixins/PageBreaksMixin.type.ts`	Replaced ambient export placeholder with concrete Shape import; updated module augmentation of AbstractGraph interface to type horizontalPageBreaks and verticalPageBreaks as Shape[] \| null instead of any[] \| null.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Review focus areas:
- Verify Shape import path is correct and available
- Confirm null guard placement doesn't inadvertently break existing callers relying on fallback behavior
- Validate that Shape[] typing accurately represents the internal page break shape structure

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is minimal and lacks required checklist items, issue reference, detailed explanation of the problem solved, and testing information as specified in the template.	Complete the PR checklist items (issue reference, scope confirmation, tests, screenshots if applicable, docs updates), provide detailed explanation of the problem being solved, and explain how the changes address it.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: improving type usage in PageBreaksMixin by replacing 'any' with 'Shape'.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch refactor/improve_PageBreakMixin_types

📜 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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

sonarqubecloud · 2025-11-24T15:12:07Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

refactor: improve types usage in PageBreaksMixin

05f10ce

Remove some any and use Shape instead.

tbouffard added the refactor Code refactoring label Nov 24, 2025

tbouffard merged commit 8b713cb into main Nov 24, 2025
12 checks passed

tbouffard deleted the refactor/improve_PageBreakMixin_types branch November 24, 2025 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: improve types usage in PageBreaksMixin #956

refactor: improve types usage in PageBreaksMixin #956

Uh oh!

tbouffard commented Nov 24, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 24, 2025 •

edited

Loading

Uh oh!

sonarqubecloud bot commented Nov 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

refactor: improve types usage in PageBreaksMixin #956

refactor: improve types usage in PageBreaksMixin #956

Uh oh!

Conversation

tbouffard commented Nov 24, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

sonarqubecloud bot commented Nov 24, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

tbouffard commented Nov 24, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 24, 2025 •

edited

Loading